From a7cae2f668220f22def673a556dcb7a0861d9fa9 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 21 Sep 2026 14:01:43 +0200 Subject: [PATCH 1/2] fix(bridge): bound disk I/O in position-encoding conversion Converting an LSP location to MCP coordinates fell through to a whole-file disk read with no cache whenever the target line wasn't already in DocumentTracker's in-memory state, and every response looped over every returned location with no cap. A hostile or malfunctioning LSP server could turn one references/goto/workspace- symbol/call-hierarchy/inlay-hints/rename response into on the order of a terabyte of disk I/O for a single MCP tool call. Line text read from disk is now memoized per (path, line) for the lifetime of one response, and read incrementally instead of loading the whole file. A per-response byte budget bounds total disk I/O across every EncodingCtx-mediated handler regardless of how many distinct files or lines are touched, charged as bytes are scanned so it cannot be bypassed by a failed read (invalid UTF-8, size-limit truncation, or a nonexistent path). References, goto-definition/ implementation/type-definition, and workspace-symbol additionally cap the number of locations/symbols normalized per response and report truncation to the caller via a new `truncated` field. --- CHANGELOG.md | 1 + crates/mcpls-core/src/bridge/state.rs | 466 +++++++++++++++++- .../src/bridge/translator/diagnostics.rs | 1 + .../mcpls-core/src/bridge/translator/dto.rs | 25 + .../src/bridge/translator/encoding_ctx.rs | 357 ++++++++++++-- .../mcpls-core/src/bridge/translator/mod.rs | 1 + .../src/bridge/translator/navigation.rs | 252 +++++++++- .../src/bridge/translator/symbols.rs | 151 ++++-- .../src/bridge/translator/testing.rs | 3 +- crates/mcpls-core/src/mcp/server.rs | 10 +- crates/mcpls-core/src/mcp/tool_surface.json | 18 +- 11 files changed, 1189 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8ef3f9a..7ea06e2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - LSP frame header parsing is now bounded on both single-line length and header count per frame, closing an unbounded-memory-growth path from a malicious or malfunctioning spawned LSP server. (#457, #463) - CodeQL workflow now triggers on `pull_request` instead of `pull_request_target`, closing a "pwn request" path where a fork PR's `build.rs`/proc-macros ran during `autobuild` with a base-repository `GITHUB_TOKEN` and could poison the default-branch Actions cache. (#464, #469) - **Workspace-roots validation now fails closed** — Breaking change: the diagnostics pump and rename/code-action edit filtering previously allowed unrestricted access when no workspace roots were configured; both now reject with no unrestricted opt-in, so embedders must call `Translator::set_workspace_roots` with real roots before serving any path-taking request. (#449) +- Position-encoding conversion no longer amplifies one LSP response into unbounded disk reads: line text is now cached and read incrementally per response, and a per-response byte budget bounds the disk I/O any navigation/references/workspace-symbol/call-hierarchy/inlay-hints/rename response can trigger; references, goto-X, and workspace-symbol are additionally capped on result count. (#474) ## [0.5.0] - 2026-09-06 diff --git a/crates/mcpls-core/src/bridge/state.rs b/crates/mcpls-core/src/bridge/state.rs index d73275d9..790f79d5 100644 --- a/crates/mcpls-core/src/bridge/state.rs +++ b/crates/mcpls-core/src/bridge/state.rs @@ -13,7 +13,7 @@ use lsp_types::{ TextDocumentItem, Uri, VersionedTextDocumentIdentifier, }; use tokio::fs; -use tokio::io::AsyncReadExt; +use tokio::io::{AsyncBufReadExt, AsyncReadExt}; use tokio::sync::{Mutex as AsyncMutex, OwnedMutexGuard}; use tokio::time::Instant; use url::Url; @@ -285,6 +285,39 @@ impl Default for ResourceLimits { } } +/// Nominal charge for a [`DocumentTracker::read_line_checked`] call whose +/// [`DocumentTracker::open_checked`] failed (path doesn't exist, isn't a +/// regular file, or already exceeds `max_file_size`) -- zero bytes were +/// actually scanned, but charging a literal `0` would let a response naming +/// many nonexistent paths (a routine, non-attacker-controlled LSP server +/// behavior -- e.g. rust-analyzer's stdlib locations without `rust-src` +/// installed) repeat that cheap-but-nonzero syscall for free against a +/// per-response I/O budget (see #474's budget-bypass follow-up). Small +/// enough to have no material effect on a legitimate response's budget +/// (~10,000 failed opens before exhausting [`DEFAULT_MAX_FILE_SIZE`]'s +/// worth of budget on their own), while still bounding the failed-open +/// amplification to the same order of magnitude as other count caps in this +/// crate. +pub const OPEN_FAILURE_CHARGE_BYTES: u64 = 4096; + +/// Outcome of [`DocumentTracker::read_line_checked`]: the requested line +/// (`None` if the file has fewer lines, doesn't exist, or otherwise +/// resolved to no usable text), plus the bytes to charge a caller +/// tracking its own I/O budget across many calls (see `EncodingCtx`'s +/// per-response disk-read budget, #474) -- not always a literal count of +/// bytes scanned (see [`OPEN_FAILURE_CHARGE_BYTES`]), but always safe to +/// charge as such. Charge this rather than assuming cost is proportional +/// to `text`'s own length -- most of the cost is the lines skipped before +/// it. +#[derive(Debug, Clone)] +pub struct LineRead { + /// The requested line's text, or `None` if the file has no such line. + pub(crate) text: Option, + /// Bytes to charge against a caller's I/O budget for this call; see + /// this type's own doc for when this isn't a literal scanned-byte count. + pub(crate) bytes_read: u64, +} + /// Tracks document state across the workspace. /// /// Every method takes `&self`: the document map and the per-path locks used @@ -824,9 +857,11 @@ impl DocumentTracker { Ok((content, mtime, size)) } - /// Reads `path`'s full content directly from disk, applying the same - /// regular-file and [`Self::check_file_size`] checks as a tracked - /// document's disk read (see [`Self::read_to_string_checked`]). + /// Reads only the 0-based `line`'th line of `path` from disk, applying + /// the same regular-file and [`Self::check_file_size`] checks as a + /// tracked document's disk read (see [`Self::read_to_string_checked`]), + /// but stopping as soon as `line` is found rather than buffering the + /// whole file just to discard everything past one line (see #474). /// /// For a document not tracked by this tracker at all -- e.g. one /// resolved only for encoding-conversion purposes, never opened for LSP @@ -834,9 +869,118 @@ impl DocumentTracker { /// all (see #427). Callers that only need best-effort text (falling back /// to `None` on any error) should treat every error here that way rather /// than surfacing it. - pub(crate) async fn read_checked(&self, path: &Path) -> Result { - let (file, meta) = self.open_checked(path).await?; - self.read_string_bounded(path, file, meta.len()).await + /// + /// [`LineRead::text`] is `None` if `path` doesn't resolve to an + /// existing, readable regular file at all (see [`Self::open_checked`]), + /// if `path` has fewer than `line + 1` lines, if the line's bytes are + /// not valid UTF-8, or if `budget` (or `max_file_size`) was exhausted + /// before a complete line could be read -- [`LineRead::bytes_read`] is + /// populated in every one of these cases (see below), never silently + /// dropped via an `Err` with no byte count. The line's trailing line + /// ending is stripped to match `str::lines`'s convention exactly: a + /// trailing `\n` is removed, and only then is one further trailing `\r` + /// also removed (a real `\r\n` terminator) -- a final line with no + /// trailing `\n` at all keeps any trailing `\r` verbatim, since it was + /// never followed by a real line terminator, same as `str::lines`. + /// + /// `budget` bounds this call's own read on top of + /// [`crate::util::bounded_read_cap`] of `max_file_size`: the actual cap + /// used is `min(bounded_read_cap(max_file_size), budget + 1)`, enforced + /// by wrapping the file handle itself in [`AsyncReadExt::take`] rather + /// than checked after the fact -- so this call physically cannot scan + /// more than one byte past `budget`, regardless of how large + /// `max_file_size` is configured (including `max_file_size = 0`, + /// meaning unlimited). The `+ 1` is the same disambiguation slack + /// `bounded_read_cap` already applies to `max_file_size`: without it, a + /// read whose remaining budget exactly equals its target line's byte + /// length (no trailing newline) is indistinguishable from one + /// genuinely truncated by the cap. A caller enforcing its own I/O + /// budget across many calls (see `EncodingCtx`'s per-response + /// disk-read budget, #474) passes its remaining allowance here and + /// charges exactly [`LineRead::bytes_read`] afterward -- always + /// available, on every outcome, so the budget can never be bypassed by + /// triggering a failure mid-scan, and never overshoots by more than + /// this one byte of slack. + /// + /// [`Self::open_checked`] failing (path doesn't exist, isn't a regular + /// file, or already exceeds `max_file_size` at stat time) is reported + /// the same way, charging [`OPEN_FAILURE_CHARGE_BYTES`] rather than a + /// literal `0` -- zero bytes were actually scanned, but an LSP server + /// routinely names paths that don't exist locally (e.g. rust-analyzer's + /// `file:///rustc//library/...` without `rust-src` installed), + /// and a literal `0` would let a response naming many such paths repeat + /// this cheap-but-nonzero syscall for free against the per-response + /// budget (see #474's budget-bypass follow-up). A real mid-read I/O + /// error (rare, not attacker-controlled by response content) is the one + /// case that still returns a genuine `Err` with no byte count. + /// + /// Also closes #427/#418's TOCTOU margin without a dedicated error: if + /// `path` grows past `max_file_size` (or past `budget`) between + /// [`Self::open_checked`]'s stat and this read completing, the capped + /// take-adapter simply runs out mid-line, which this method detects + /// (`buf` doesn't end in the expected `\n`) and reports as `None` rather + /// than returning a truncated line as if it were complete. + pub(crate) async fn read_line_checked( + &self, + path: &Path, + line: u32, + budget: u64, + ) -> Result { + let Ok((file, _meta)) = self.open_checked(path).await else { + return Ok(LineRead { + text: None, + bytes_read: OPEN_FAILURE_CHARGE_BYTES, + }); + }; + let max = self.limits.max_file_size; + // `+1` slack on `budget`, same trick `bounded_read_cap` already + // applies to `max_file_size`: without it, a read whose remaining + // budget exactly equals its target line's byte length (no trailing + // newline) is indistinguishable from one truncated by the cap, and + // was misreported as truncated (see #474's correctness-gate fix). + let cap = bounded_read_cap(max).min(budget.saturating_add(1)); + let mut reader = tokio::io::BufReader::new(file.take(cap)); + let io_err = |e: std::io::Error| Error::FileIo { + path: path.to_path_buf(), + source: e, + }; + + let mut buf = Vec::new(); + let mut bytes_read: u64 = 0; + let mut current_line = 0u32; + loop { + buf.clear(); + let n = reader.read_until(b'\n', &mut buf).await.map_err(io_err)?; + bytes_read += n as u64; + if n == 0 { + // No complete line left to return either way; bytes scanned + // are still reported so the caller can charge them. + return Ok(LineRead { + text: None, + bytes_read, + }); + } + if current_line == line { + let truncated_by_cap = bytes_read >= cap && buf.last() != Some(&b'\n'); + if truncated_by_cap { + return Ok(LineRead { + text: None, + bytes_read, + }); + } + if buf.last() == Some(&b'\n') { + buf.pop(); + if buf.last() == Some(&b'\r') { + buf.pop(); + } + } + return Ok(LineRead { + text: String::from_utf8(buf).ok(), + bytes_read, + }); + } + current_line += 1; + } } /// Per-server sync phase of `ensure_open`: sends `didOpen`, `didChange`, @@ -2899,4 +3043,312 @@ mod tests { Err(Error::FileSizeLimitExceeded { size: 11, max: 10 }) )); } + + /// Regression for #474: `read_line_checked` must stop reading (and + /// UTF-8-decoding) once it has the requested line, not buffer/validate + /// the rest of the file. The file's second line is invalid UTF-8, which + /// would fail a whole-file read (as the pre-#474 `read_checked` + + /// `.lines().nth(...)` path did); reading line 0 must still succeed. + #[tokio::test] + async fn test_read_line_checked_does_not_read_past_target_line() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("partial.rs"); + let mut content = b"hello\n".to_vec(); + content.extend_from_slice(&[0xFF, 0xFE]); + content.push(b'\n'); + std::fs::write(&path, &content).unwrap(); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let line = tracker.read_line_checked(&path, 0, u64::MAX).await.unwrap(); + assert_eq!(line.text.as_deref(), Some("hello")); + } + + /// Regression for M3: an off-by-one in `current_line` (e.g. returning + /// line `N + 1` for `N`) would ship green if every test used line 0. + /// Exercises a non-zero target line on a multi-line fixture. + #[tokio::test] + async fn test_read_line_checked_returns_requested_non_zero_line() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("multi.rs"); + std::fs::write(&path, "first\nsecond\nthird\nfourth\n").unwrap(); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + assert_eq!( + tracker + .read_line_checked(&path, 2, u64::MAX) + .await + .unwrap() + .text + .as_deref(), + Some("third") + ); + } + + /// `read_line_checked` must report `Ok(None)`, not an error, when `line` + /// is past the file's last line -- distinguishing "file has fewer lines + /// than requested" from an actual read failure. + #[tokio::test] + async fn test_read_line_checked_returns_none_past_last_line() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("short.rs"); + std::fs::write(&path, "only one line").unwrap(); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + assert_eq!( + tracker + .read_line_checked(&path, 5, u64::MAX) + .await + .unwrap() + .text, + None + ); + } + + /// A requested line with no trailing `\n` at all (the file's only line, + /// never terminated) must still be returned -- distinct from + /// `test_read_line_checked_returns_none_past_last_line`, which requests a + /// line number past this same kind of file instead of the line itself. + #[tokio::test] + async fn test_read_line_checked_reads_last_line_without_trailing_newline() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("no_newline.rs"); + std::fs::write(&path, "only one line").unwrap(); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + assert_eq!( + tracker + .read_line_checked(&path, 0, u64::MAX) + .await + .unwrap() + .text + .as_deref(), + Some("only one line") + ); + } + + #[tokio::test] + async fn test_read_line_checked_empty_file_returns_none() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("empty.rs"); + std::fs::write(&path, "").unwrap(); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + assert_eq!( + tracker + .read_line_checked(&path, 0, u64::MAX) + .await + .unwrap() + .text, + None + ); + } + + /// `read_until(b'\n', ..)` splits lines on `\n` alone, so a `\r` ahead of + /// it is left in `buf` until the trailing-separator strip loop removes + /// it -- pins that CRLF-terminated lines come out identical to LF-only + /// ones. + #[tokio::test] + async fn test_read_line_checked_strips_crlf_line_ending() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("crlf.rs"); + std::fs::write(&path, "first\r\nsecond\r\n").unwrap(); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + assert_eq!( + tracker + .read_line_checked(&path, 0, u64::MAX) + .await + .unwrap() + .text + .as_deref(), + Some("first") + ); + assert_eq!( + tracker + .read_line_checked(&path, 1, u64::MAX) + .await + .unwrap() + .text + .as_deref(), + Some("second") + ); + } + + /// Regression for M2: `str::lines` strips at most one trailing `\r` per + /// line, not every trailing `\r`, and only when it precedes an actual + /// `\n` terminator -- a final, untermined line keeps a trailing `\r` + /// verbatim. Uses `str::lines` itself as the oracle on the exact inputs + /// that distinguish these from a naive "strip every trailing `\r`/`\n`" + /// implementation. + #[tokio::test] + async fn test_read_line_checked_matches_str_lines_crlf_semantics() { + let dir = TempDir::new().unwrap(); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + + let double_cr = "abc\r\r\n"; + let path_a = dir.path().join("double_cr.rs"); + std::fs::write(&path_a, double_cr).unwrap(); + assert_eq!( + tracker + .read_line_checked(&path_a, 0, u64::MAX) + .await + .unwrap() + .text + .as_deref(), + double_cr.lines().next() + ); + + let trailing_cr_no_newline = "abc\r"; + let path_b = dir.path().join("trailing_cr_no_newline.rs"); + std::fs::write(&path_b, trailing_cr_no_newline).unwrap(); + assert_eq!( + tracker + .read_line_checked(&path_b, 0, u64::MAX) + .await + .unwrap() + .text + .as_deref(), + trailing_cr_no_newline.lines().next() + ); + } + + /// Regression for the `bounded_read_cap` off-by-one: a file whose size + /// is exactly `max_file_size` must not be misreported as oversized when + /// a request (for a line past the file's content) forces a full read to + /// EOF. The cap is `max_file_size + 1` precisely so this exact-boundary + /// case is distinguishable from a genuinely oversized file. + #[tokio::test] + async fn test_read_line_checked_exact_max_file_size_reads_to_eof_without_error() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("exact.rs"); + let content = "a".repeat(20); + std::fs::write(&path, &content).unwrap(); + + let limits = ResourceLimits { + max_documents: 100, + max_file_size: 20, + }; + let tracker = DocumentTracker::new(limits, HashMap::new()); + + assert_eq!( + tracker + .read_line_checked(&path, 0, u64::MAX) + .await + .unwrap() + .text + .as_deref(), + Some(content.as_str()) + ); + assert_eq!( + tracker + .read_line_checked(&path, 1, u64::MAX) + .await + .unwrap() + .text, + None, + "a line past an exact-max_file_size file's only line must read to EOF cleanly, not \ + be misreported as truncated" + ); + } + + /// Regression for the S1 budget-bypass fix: `budget` must physically + /// bound the read (via the take-adapter), not just gate whether a read + /// is attempted -- a read that starts with budget left must still stop + /// at exactly that many bytes, never at the full `max_file_size`. + /// Distinguishes this from `bounded_read_cap(max_file_size)` alone by + /// using a `budget` far smaller than `max_file_size`. + #[tokio::test] + async fn test_read_line_checked_bounds_read_by_budget_not_just_max_file_size() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("budget.rs"); + std::fs::write(&path, "a".repeat(1000)).unwrap(); + + let limits = ResourceLimits { + max_documents: 100, + max_file_size: 1000, + }; + let tracker = DocumentTracker::new(limits, HashMap::new()); + + let read = tracker.read_line_checked(&path, 0, 10).await.unwrap(); + assert_eq!( + read.text, None, + "a single line far longer than the budget must not be returned as if complete" + ); + assert_eq!( + read.bytes_read, 11, + "the read must stop at exactly the budget's +1 slack (see the correctness-gate fix \ + below), not at max_file_size" + ); + } + + /// Regression for a correctness-gate finding: `cap`'s `budget` component + /// needs the same `+1` disambiguation slack `bounded_read_cap` already + /// applies to `max_file_size` -- without it, a read whose remaining + /// budget exactly equals its target line's byte length (no trailing + /// newline) is indistinguishable from one genuinely truncated by the + /// cap, and was misreported as truncated (`text: None`) even though the + /// read fully succeeded. + #[tokio::test] + async fn test_read_line_checked_exact_budget_match_on_unterminated_line_not_truncated() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("exact_budget.rs"); + let content = "twelve chars"; + std::fs::write(&path, content).unwrap(); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let read = tracker + .read_line_checked(&path, 0, content.len() as u64) + .await + .unwrap(); + assert_eq!( + read.text.as_deref(), + Some(content), + "budget exactly matching the line's byte length must not be misreported as truncated" + ); + assert_eq!(read.bytes_read, content.len() as u64); + } + + /// Regression for the S1 budget-bypass fix: an invalid-UTF-8 line (the + /// realistic attack shape -- a `.rlib`/image/pack file under + /// `max_file_size`) must still report an accurate `bytes_read` on + /// `LineRead::text == None`, not lose it down an `Err` path with no byte + /// count -- that loss is exactly what let a hostile response scan + /// unlimited bytes while charging the per-response budget zero. + #[tokio::test] + async fn test_read_line_checked_reports_bytes_read_for_invalid_utf8_line() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("invalid_utf8.rs"); + let mut content = vec![0xFFu8, 0xFE, 0xFD]; + content.push(b'\n'); + std::fs::write(&path, &content).unwrap(); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let read = tracker.read_line_checked(&path, 0, u64::MAX).await.unwrap(); + assert_eq!(read.text, None); + assert_eq!( + read.bytes_read, + content.len() as u64, + "bytes scanned must be reported even though the line wasn't valid UTF-8" + ); + } + + /// Regression for the open-failure-charge fix: a path that doesn't + /// exist (the realistic, non-attacker case -- e.g. an LSP server naming + /// a stdlib location not present locally) must resolve to `Ok(None)`, + /// not `Err`, and must charge the small nominal + /// `OPEN_FAILURE_CHARGE_BYTES` amount rather than `0` (which would let + /// a response repeat this for free) or the full budget (the previous + /// round's regression, which zeroed the whole per-response budget on + /// the very first such location). + #[tokio::test] + async fn test_read_line_checked_charges_nominal_amount_for_nonexistent_path() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("does_not_exist.rs"); + + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + // A nonexistent path must resolve to Ok(None), not Err. + let read = tracker.read_line_checked(&path, 0, u64::MAX).await.unwrap(); + assert_eq!(read.text, None); + assert_eq!(read.bytes_read, OPEN_FAILURE_CHARGE_BYTES); + } } diff --git a/crates/mcpls-core/src/bridge/translator/diagnostics.rs b/crates/mcpls-core/src/bridge/translator/diagnostics.rs index 057dab10..6105cf53 100644 --- a/crates/mcpls-core/src/bridge/translator/diagnostics.rs +++ b/crates/mcpls-core/src/bridge/translator/diagnostics.rs @@ -243,6 +243,7 @@ impl Translator { tracker: tracker.clone(), // Never read here -- see `EMPTY_WORKSPACE_ROOTS`'s doc. workspace_roots: EMPTY_WORKSPACE_ROOTS.clone(), + line_cache: super::encoding_ctx::new_line_cache(), }; let mut result = Vec::with_capacity(diag_info.diagnostics.len()); for d in &diag_info.diagnostics { diff --git a/crates/mcpls-core/src/bridge/translator/dto.rs b/crates/mcpls-core/src/bridge/translator/dto.rs index 7853989a..033ea963 100644 --- a/crates/mcpls-core/src/bridge/translator/dto.rs +++ b/crates/mcpls-core/src/bridge/translator/dto.rs @@ -97,6 +97,12 @@ pub struct HoverResult { pub struct DefinitionResult { /// Locations of the definition. pub locations: Vec, + /// Whether `locations` was capped below the LSP server's full response + /// (see `MAX_NORMALIZED_LOCATIONS`, #474) -- if `true`, more locations + /// exist than are returned here. Omitted (defaults to `false`) when + /// serialized. + #[serde(default, skip_serializing_if = "is_false")] + pub truncated: bool, } /// Result of a references request. @@ -104,6 +110,12 @@ pub struct DefinitionResult { pub struct ReferencesResult { /// Locations of all references. pub locations: Vec, + /// Whether `locations` was capped below the LSP server's full response + /// (see `MAX_NORMALIZED_LOCATIONS`, #474) -- if `true`, more references + /// exist than are returned here. Omitted (defaults to `false`) when + /// serialized. + #[serde(default, skip_serializing_if = "is_false")] + pub truncated: bool, } /// Diagnostic severity. @@ -276,6 +288,13 @@ pub struct WorkspaceSymbol { pub struct WorkspaceSymbolResult { /// List of symbols found. pub symbols: Vec, + /// Whether more symbols matched than are returned in `symbols` -- set + /// whenever any are dropped, whether by the caller's own smaller + /// `limit` or by the server-side maximum it's clamped to (see + /// `MAX_NORMALIZED_LOCATIONS`, #474); this does not distinguish which of + /// the two caused it. Omitted (defaults to `false`) when serialized. + #[serde(default, skip_serializing_if = "is_false")] + pub truncated: bool, } /// A single code action. @@ -454,6 +473,12 @@ pub struct SignatureHelpResult { pub struct LocationsResult { /// Locations found. pub locations: Vec, + /// Whether `locations` was capped below the LSP server's full response + /// (see `MAX_NORMALIZED_LOCATIONS`, #474) -- if `true`, more locations + /// exist than are returned here. Omitted (defaults to `false`) when + /// serialized. + #[serde(default, skip_serializing_if = "is_false")] + pub truncated: bool, } /// A single inlay hint entry. diff --git a/crates/mcpls-core/src/bridge/translator/encoding_ctx.rs b/crates/mcpls-core/src/bridge/translator/encoding_ctx.rs index 9e371b52..ab7b6430 100644 --- a/crates/mcpls-core/src/bridge/translator/encoding_ctx.rs +++ b/crates/mcpls-core/src/bridge/translator/encoding_ctx.rs @@ -1,13 +1,82 @@ //! Per-response position/range encoding conversion between MCP's 1-based //! UTF-16 columns and an LSP server's negotiated encoding. -use std::path::PathBuf; -use std::sync::Arc; +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex as StdMutex}; use super::dto::{Position2D, Range}; -use crate::bridge::DocumentTracker; use crate::bridge::encoding::{PositionEncoding, lsp_to_mcp_position, mcp_to_lsp_position}; -use crate::bridge::state::uri_to_path; +use crate::bridge::state::{DEFAULT_MAX_FILE_SIZE, uri_to_path}; +use crate::bridge::{DocumentTracker, lock_std}; + +/// Total bytes [`read_line_text`]'s disk-read fallback (via +/// [`DocumentTracker::read_line_checked`]) may scan across one +/// `EncodingCtx`'s whole lifetime (one MCP response), independent of how +/// many distinct `(path, line)` lookups that spans. +/// +/// [`LineCacheState::entries`] alone caps repeats of the *same* line, but a +/// response naming enough distinct lines (e.g. `references` results spread +/// across a large file, or several call-hierarchy/inlay-hint/workspace-edit +/// locations) could still add up to an unbounded amount of scanning even +/// with that cache and [`super::navigation::MAX_NORMALIZED_LOCATIONS`]'s +/// count cap in place (see #474's follow-up). Every conversion that reaches +/// disk goes through [`read_line_text`], so charging this single budget +/// there caps every `EncodingCtx`-mediated handler uniformly -- `to_lsp`, +/// `to_mcp`, `normalize_range`, `denormalize_range` -- with no per-handler +/// cap needed. +/// +/// Set to four times the default single-file read bound: enough slack for a +/// legitimate response touching a handful of large files, while still +/// bounding a hostile response to double-digit MiB of I/O rather than the +/// unbounded (or count-cap x `max_file_size`) amount possible without it. +/// This is a fixed constant, not derived from the tracker's *configured* +/// `ResourceLimits::max_file_size` -- deliberately: `read_line_checked`'s +/// `budget` parameter always caps an individual read to +/// `min(bounded_read_cap(configured_max_file_size), remaining_budget)`, so a +/// larger configured `max_file_size` (including `0`, meaning unlimited) +/// only widens what *one* read is theoretically allowed to scan before +/// finding its line, never what it can actually charge against this +/// response-wide budget -- the physical cap always wins. +const MAX_LINE_READ_BYTES_PER_RESPONSE: u64 = 4 * DEFAULT_MAX_FILE_SIZE; + +/// [`EncodingCtx::line_cache`]'s guarded state: the per-`(path, line)` +/// memoization table plus the shared disk-read byte budget both are checked +/// and charged against (see [`MAX_LINE_READ_BYTES_PER_RESPONSE`]). +#[derive(Debug)] +pub(super) struct LineCacheState { + /// Memoized line text keyed by `(path, 0-based line)`, `None` meaning + /// "resolved to no such line". Populated for both the tracker-hit and + /// disk-read paths (see [`read_line_text`]). + pub(super) entries: HashMap<(PathBuf, u32), Option>, + /// Remaining disk-read byte allowance for this response; see + /// [`MAX_LINE_READ_BYTES_PER_RESPONSE`]. + bytes_remaining: u64, + /// Whether the once-per-response budget-exhausted warning has already + /// been logged, so a response with many post-exhaustion lookups logs + /// once rather than once per lookup. + budget_exhausted_logged: bool, +} + +impl LineCacheState { + fn new() -> Self { + Self { + entries: HashMap::new(), + bytes_remaining: MAX_LINE_READ_BYTES_PER_RESPONSE, + budget_exhausted_logged: false, + } + } +} + +/// [`EncodingCtx::line_cache`]'s field type. +type LineCache = Arc>; + +/// Builds a fresh, empty [`LineCache`] for a new [`EncodingCtx`] -- used by +/// every construction site so the budget/cache initialization can't drift +/// between them. +pub(super) fn new_line_cache() -> LineCache { + Arc::new(StdMutex::new(LineCacheState::new())) +} /// Per-response encoding context: the negotiated [`PositionEncoding`] of the /// LSP server that produced a response, used to convert every @@ -32,32 +101,112 @@ pub(super) struct EncodingCtx { /// navigation result -- see `crate::bridge::uri_in_workspace_roots`'s /// docs for why filtering is deliberately not done here. pub(super) workspace_roots: Arc>, + /// Memoizes [`read_line_text`]'s result (both the tracker hit and the + /// disk-read fallback) per `(path, line)` for the lifetime of this + /// context, and tracks the shared disk-read byte budget -- one + /// `EncodingCtx` is built per MCP response (see + /// [`Translator::encoding_ctx`](super::Translator::encoding_ctx)), so + /// this bounds a response that reconverts the same file/line many times + /// (e.g. `references` results clustered in one file) to a single lookup + /// per distinct line, and caps the response's total disk-read I/O + /// regardless of how many distinct lines it touches (see #474). + pub(super) line_cache: LineCache, } /// Text of the 0-based `line`'th line of the file at `uri`, or `None` if it -/// cannot be resolved to a path, read, or has no such line. +/// cannot be resolved to a path, read, has no such line, or the response's +/// disk-read budget ([`MAX_LINE_READ_BYTES_PER_RESPONSE`]) is exhausted. /// /// Only ever consulted when the negotiated encoding is not UTF-16 (see -/// [`EncodingCtx::to_lsp`]/[`EncodingCtx::to_mcp`]). Checks `tracker` first -/// (in-memory, no I/O) -- this is by construction both cheaper and more -/// correct than disk for any document mcpls has opened, since it is exactly -/// the text the server was told about, so it can't diverge from the -/// server's own view even if the file has since been edited on disk (see -/// #290 S1). Only a document `tracker` has never seen falls through to -/// [`DocumentTracker::read_checked`], which applies the same -/// `ResourceLimits::max_file_size` and regular-file gate as any tracked -/// document's disk read (see #427) rather than an unbounded read. -async fn read_line_text( - uri: &lsp_types::Uri, - line: u32, - tracker: &DocumentTracker, -) -> Option { +/// [`EncodingCtx::to_lsp`]/[`EncodingCtx::to_mcp`]). Every outcome -- +/// tracker hit, disk hit, or "no such line" -- is memoized in +/// `ctx.line_cache` per `(path, line)`, so a response reconverting the same +/// line more than once pays for `ctx.tracker.line_text`'s lock/scan or +/// [`DocumentTracker::read_line_checked`]'s disk read only the first time. +/// +/// On a cache miss, checks `ctx.tracker` first (in-memory) -- correct even +/// when cached, since it is exactly the text the server was told about and +/// can't diverge from the server's own view within one response's lifetime +/// (see #290 S1: that concern is about disk-vs-tracker divergence across +/// requests, not within one). Only a document the tracker has never seen +/// falls through to [`DocumentTracker::read_line_checked`], which applies +/// the same `ResourceLimits::max_file_size` and regular-file gate as any +/// tracked document's disk read (see #427) while reading only up to the +/// requested line rather than the whole file (see #474) -- gated by the +/// per-response byte budget so that no single response can rack up +/// unbounded disk I/O by naming enough distinct lines. +async fn read_line_text(uri: &lsp_types::Uri, line: u32, ctx: &EncodingCtx) -> Option { let path = uri_to_path(uri)?; - if let Some(text) = tracker.line_text(&path, line) { - return Some(text); + let key = (path.clone(), line); + + if let Some(cached) = lock_std(&ctx.line_cache).entries.get(&key) { + return cached.clone(); + } + + let text = if let Some(text) = ctx.tracker.line_text(&path, line) { + Some(text) + } else { + disk_read_line_budgeted(&path, line, ctx).await + }; + + lock_std(&ctx.line_cache).entries.insert(key, text.clone()); + text +} + +/// [`read_line_text`]'s disk-read fallback, charging the bytes +/// [`DocumentTracker::read_line_checked`] scans against `ctx.line_cache`'s +/// shared [`MAX_LINE_READ_BYTES_PER_RESPONSE`] budget. Once exhausted, no +/// further disk reads are attempted for the rest of this response -- every +/// subsequent budget-gated lookup returns `None` immediately, logging a +/// single `warn!` the first time that happens. +/// +/// The remaining budget is passed *into* the read itself +/// (`read_line_checked`'s `budget` parameter), which physically bounds how +/// many bytes that call can scan -- so unlike charging only on success, +/// this can't be bypassed by a read that ends in a content-shaped failure +/// (invalid UTF-8 at the target line, a truncation-by-cap, or a path that +/// doesn't resolve via `open_checked` at all -- e.g. an LSP server naming a +/// stdlib path not present locally): `LineRead` reports `bytes_read` on +/// every one of those outcomes too (a small nominal charge, not a literal +/// `0`, for the `open_checked`-failure case -- see +/// `state::OPEN_FAILURE_CHARGE_BYTES`), and this function always charges +/// exactly that. Only a genuine mid-read I/O error (rare, not +/// attacker-controlled by response content) has no byte count available; +/// that one case fails safe by charging this call's whole budget slice +/// rather than leaving it unaccounted (see #474's S1 budget-bypass fix). +async fn disk_read_line_budgeted(path: &Path, line: u32, ctx: &EncodingCtx) -> Option { + let budget = { + let mut state = lock_std(&ctx.line_cache); + if state.bytes_remaining != 0 { + state.bytes_remaining + } else { + let already_logged = state.budget_exhausted_logged; + state.budget_exhausted_logged = true; + drop(state); + if !already_logged { + tracing::warn!( + path = %path.display(), + budget_bytes = MAX_LINE_READ_BYTES_PER_RESPONSE, + "per-response disk-read budget exhausted; further position conversions \ + requiring a disk read in this response will pass columns through \ + unconverted" + ); + } + return None; + } + }; + + if let Ok(read) = ctx.tracker.read_line_checked(path, line, budget).await { + let mut state = lock_std(&ctx.line_cache); + state.bytes_remaining = state.bytes_remaining.saturating_sub(read.bytes_read); + drop(state); + read.text + } else { + let mut state = lock_std(&ctx.line_cache); + state.bytes_remaining = state.bytes_remaining.saturating_sub(budget); + drop(state); + None } - let content = tracker.read_checked(&path).await.ok()?; - content.lines().nth(line as usize).map(str::to_string) } impl EncodingCtx { @@ -100,7 +249,7 @@ impl EncodingCtx { let line_text = if self.encoding == PositionEncoding::Utf16 { None } else { - let text = read_line_text(uri, line.saturating_sub(1), &self.tracker).await; + let text = read_line_text(uri, line.saturating_sub(1), self).await; if text.is_none() { tracing::warn!( uri = uri.as_ref(), @@ -125,7 +274,7 @@ impl EncodingCtx { let line_text = if self.encoding == PositionEncoding::Utf16 { None } else { - let text = read_line_text(uri, pos.line, &self.tracker).await; + let text = read_line_text(uri, pos.line, self).await; if text.is_none() { tracing::warn!( uri = uri.as_ref(), @@ -277,15 +426,20 @@ mod tests { fs::write(&path, "a".repeat(200)).unwrap(); let uri = path_to_uri(&path).unwrap(); - let tracker = DocumentTracker::new( - ResourceLimits { - max_documents: 100, - max_file_size: 50, - }, - HashMap::new(), - ); + let ctx = EncodingCtx { + encoding: PositionEncoding::Utf8, + tracker: Arc::new(DocumentTracker::new( + ResourceLimits { + max_documents: 100, + max_file_size: 50, + }, + HashMap::new(), + )), + workspace_roots: Arc::new(Vec::new()), + line_cache: new_line_cache(), + }; assert!( - read_line_text(&uri, 0, &tracker).await.is_none(), + read_line_text(&uri, 0, &ctx).await.is_none(), "must refuse to return content from a file over max_file_size" ); } @@ -314,6 +468,7 @@ mod tests { encoding: PositionEncoding::Utf8, tracker, workspace_roots: Arc::new(Vec::new()), + line_cache: new_line_cache(), }; let lsp_pos = ctx.to_lsp(&uri, 1, 3).await; assert_eq!( @@ -366,4 +521,140 @@ mod tests { "must convert against b.rs's own content" ); } + + /// Regression for #474: a single `EncodingCtx` must memoize + /// [`read_line_text`]'s disk-read fallback per `(path, line)`, so a + /// response that reconverts the same untracked file's line more than + /// once (e.g. several `references` locations on one line) reads disk + /// only the first time. Proven by mutating the file between two lookups + /// through the same `ctx`: if the second lookup re-read disk, it would + /// observe the new content instead of the cached one. + #[tokio::test] + async fn test_read_line_text_caches_disk_read_per_path_line() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("cached.rs"); + fs::write(&path, "hello").unwrap(); + let uri = path_to_uri(&path).unwrap(); + + let ctx = test_ctx_with(PositionEncoding::Utf8); + assert_eq!( + read_line_text(&uri, 0, &ctx).await.as_deref(), + Some("hello") + ); + + fs::write(&path, "héllo").unwrap(); + assert_eq!( + read_line_text(&uri, 0, &ctx).await.as_deref(), + Some("hello"), + "must reuse the first lookup's cached result instead of re-reading disk" + ); + } + + /// Regression for S1: the per-`(path, line)` cache alone doesn't bound a + /// response naming enough *distinct* lines/files -- `read_line_text` + /// must also stop performing disk reads once the shared per-response + /// byte budget is spent, refusing further lookups rather than letting + /// each new distinct key add unbounded I/O. + #[tokio::test] + async fn test_read_line_text_stops_disk_reads_once_budget_exhausted() { + let dir = TempDir::new().unwrap(); + let path_a = dir.path().join("a.rs"); + fs::write(&path_a, "hello\n").unwrap(); + let uri_a = path_to_uri(&path_a).unwrap(); + + let path_b = dir.path().join("b.rs"); + fs::write(&path_b, "world\n").unwrap(); + let uri_b = path_to_uri(&path_b).unwrap(); + + let ctx = test_ctx_with(PositionEncoding::Utf8); + // Exactly enough budget for the first read ("hello\n" is 6 bytes) to + // complete, but nothing left after. + lock_std(&ctx.line_cache).bytes_remaining = 6; + + assert_eq!( + read_line_text(&uri_a, 0, &ctx).await.as_deref(), + Some("hello") + ); + assert_eq!(lock_std(&ctx.line_cache).bytes_remaining, 0); + + // A second, distinct (path, line) lookup must now be refused. + assert_eq!(read_line_text(&uri_b, 0, &ctx).await, None); + } + + /// Regression for the S1 budget-bypass fix: a read whose remaining + /// budget is smaller than the line it's scanning for must stop at + /// exactly the budget (never returning the truncated text as if it + /// were complete), and must still charge exactly what it scanned -- + /// proven by draining the budget to zero rather than leaving any + /// unaccounted. + #[tokio::test] + async fn test_read_line_text_bounds_read_by_remaining_budget() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("long_line.rs"); + fs::write(&path, "a".repeat(1000)).unwrap(); + let uri = path_to_uri(&path).unwrap(); + + let ctx = test_ctx_with(PositionEncoding::Utf8); + lock_std(&ctx.line_cache).bytes_remaining = 10; + + assert_eq!( + read_line_text(&uri, 0, &ctx).await, + None, + "a line far longer than the remaining budget must not be returned" + ); + assert_eq!( + lock_std(&ctx.line_cache).bytes_remaining, + 0, + "the physically-capped read must charge (at most one byte over) the budget it was \ + given, not overshoot to max_file_size" + ); + } + + /// Regression for the S1 budget-bypass fix (security re-audit): an + /// invalid-UTF-8 line -- the realistic attack shape (a `.rlib`, image, + /// or pack file under `max_file_size`) -- must still charge the shared + /// per-response budget for the bytes actually scanned, not leave it + /// unaccounted because the line failed to decode. Before this fix, this + /// exact case charged zero, letting a hostile response repeat it over + /// enough distinct `(path, line)` keys to restore unbounded scanning. + #[tokio::test] + async fn test_read_line_text_charges_budget_even_when_line_is_invalid_utf8() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("invalid_utf8.rs"); + let mut content = vec![0xFFu8, 0xFE, 0xFD]; + content.push(b'\n'); + fs::write(&path, &content).unwrap(); + let uri = path_to_uri(&path).unwrap(); + + let ctx = test_ctx_with(PositionEncoding::Utf8); + assert_eq!(read_line_text(&uri, 0, &ctx).await, None); + assert_eq!( + lock_std(&ctx.line_cache).bytes_remaining, + MAX_LINE_READ_BYTES_PER_RESPONSE - content.len() as u64, + "the budget must be charged for the bytes scanned even though the line was not \ + valid UTF-8" + ); + } + + /// Regression for the open-failure-charge fix: an LSP server routinely + /// names a path that doesn't exist locally (e.g. rust-analyzer's + /// `file:///rustc//library/...` stdlib locations without + /// `rust-src` installed) -- a completely normal, non-attacker scenario. + /// This must charge only the small nominal `OPEN_FAILURE_CHARGE_BYTES` + /// amount, not the previous round's regression of zeroing the *entire* + /// remaining per-response budget on the very first such location. + #[tokio::test] + async fn test_read_line_text_charges_nominal_amount_for_nonexistent_path() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("rustc_stdlib_without_rust_src.rs"); + let uri = path_to_uri(&path).unwrap(); + + let ctx = test_ctx_with(PositionEncoding::Utf8); + assert_eq!(read_line_text(&uri, 0, &ctx).await, None); + assert_eq!( + lock_std(&ctx.line_cache).bytes_remaining, + MAX_LINE_READ_BYTES_PER_RESPONSE - crate::bridge::state::OPEN_FAILURE_CHARGE_BYTES, + "a nonexistent path must charge only the small nominal amount, not the whole budget" + ); + } } diff --git a/crates/mcpls-core/src/bridge/translator/mod.rs b/crates/mcpls-core/src/bridge/translator/mod.rs index ef2be5b0..867d4b82 100644 --- a/crates/mcpls-core/src/bridge/translator/mod.rs +++ b/crates/mcpls-core/src/bridge/translator/mod.rs @@ -289,6 +289,7 @@ impl Translator { encoding: self.position_encoding_for(server_id), tracker: self.document_tracker.clone(), workspace_roots: self.workspace_roots.clone(), + line_cache: encoding_ctx::new_line_cache(), } } diff --git a/crates/mcpls-core/src/bridge/translator/navigation.rs b/crates/mcpls-core/src/bridge/translator/navigation.rs index aa81f5f5..99d67bb0 100644 --- a/crates/mcpls-core/src/bridge/translator/navigation.rs +++ b/crates/mcpls-core/src/bridge/translator/navigation.rs @@ -87,9 +87,26 @@ fn definition_link_to_location(link: lsp_types::DefinitionLink) -> lsp_types::Lo } } +/// Hard cap on the number of `Location`s/symbols a single call normalizes +/// (`goto`, `references`, `workspace_symbol_search`). Without a limit, a +/// response naming an unbounded number of locations turns one MCP tool call +/// into an unbounded number of range conversions -- each one a potential +/// disk read on a cache miss -- letting a hostile or misbehaving LSP server +/// amplify one request into massive I/O (see #474). Applied before +/// normalization, not after, so it bounds the work actually done rather +/// than just the size of the returned list. Also used by +/// `Translator::handle_workspace_symbol` to clamp its caller-supplied +/// `limit`, which otherwise has no upper bound of its own. +pub(super) const MAX_NORMALIZED_LOCATIONS: usize = 10_000; + /// Converts raw LSP locations into MCP-facing `Location` values, normalizing /// each range into the caller's 1-based coordinate space. /// +/// Truncates to [`MAX_NORMALIZED_LOCATIONS`] first -- see its doc. Logs a +/// single `warn!` when that truncation actually drops locations, so a +/// response silently capped below what the LSP server reported is at least +/// visible in logs (see #474). +/// /// Deliberately not filtered to workspace roots: unlike a write-bearing /// `WorkspaceEdit` (see `edits.rs`), a goto-X/references location is /// read-only, and legitimate results routinely point outside the workspace @@ -98,7 +115,19 @@ fn definition_link_to_location(link: lsp_types::DefinitionLink) -> lsp_types::Lo /// the path this location names still goes through the inbound /// `validate_path_against_roots` gate (`mcp/server.rs`), which fails closed, /// so the untrusted-URI concern is already covered downstream. -async fn lsp_locations_to_mcp(locs: Vec, ctx: &EncodingCtx) -> Vec { +async fn lsp_locations_to_mcp( + mut locs: Vec, + ctx: &EncodingCtx, +) -> NormalizedLocations { + let truncated = locs.len() > MAX_NORMALIZED_LOCATIONS; + if truncated { + tracing::warn!( + reported = locs.len(), + cap = MAX_NORMALIZED_LOCATIONS, + "LSP response location count exceeds MAX_NORMALIZED_LOCATIONS; truncating" + ); + } + locs.truncate(MAX_NORMALIZED_LOCATIONS); let mut locations = Vec::with_capacity(locs.len()); for loc in locs { locations.push(Location { @@ -107,7 +136,20 @@ async fn lsp_locations_to_mcp(locs: Vec, ctx: &EncodingCtx) out_of_workspace: ctx.is_out_of_workspace(&loc.uri), }); } - locations + NormalizedLocations { + locations, + truncated, + } +} + +/// [`lsp_locations_to_mcp`]'s result: the normalized locations plus whether +/// [`MAX_NORMALIZED_LOCATIONS`] actually dropped any of the LSP server's +/// reported locations -- surfaced to the MCP caller via each result DTO's +/// `truncated` field, since `references`'/goto-X's tool descriptions +/// otherwise imply a complete result (see #474). +struct NormalizedLocations { + locations: Vec, + truncated: bool, } /// The two response shapes shared by `textDocument/definition`, @@ -163,7 +205,7 @@ impl GotoResponse for lsp_types::TypeDefinitionResponse { async fn goto_response_to_locations( response: Option, ctx: &EncodingCtx, -) -> Vec { +) -> NormalizedLocations { let lsp_locs = match response.map(GotoResponse::into_kind) { Some(GotoKind::Definition(def)) => definition_to_locations(def), Some(GotoKind::DefinitionLinkList(links)) => { @@ -390,7 +432,7 @@ impl Translator { position: Position, tool: ToolKind, capability: Capability, - ) -> Result> + ) -> Result where R: lsp_types::Request>, R::Params: GotoParams, @@ -428,7 +470,10 @@ impl Translator { file_path: String, position: Position, ) -> Result { - let locations = self + let NormalizedLocations { + locations, + truncated, + } = self .handle_goto::( &file_path, position, @@ -437,7 +482,10 @@ impl Translator { ) .await?; - Ok(DefinitionResult { locations }) + Ok(DefinitionResult { + locations, + truncated, + }) } /// Handle references request. @@ -483,12 +531,15 @@ impl Translator { .await?; let locations = response.unwrap_or_default(); - let result_locations = lsp_locations_to_mcp(locations, &ctx).await; - let result = ReferencesResult { - locations: result_locations, - }; - - Ok(result) + let NormalizedLocations { + locations, + truncated, + } = lsp_locations_to_mcp(locations, &ctx).await; + + Ok(ReferencesResult { + locations, + truncated, + }) } /// Handle go-to-implementation request (`textDocument/implementation`). @@ -506,7 +557,10 @@ impl Translator { file_path: String, position: Position, ) -> Result { - let locations = self + let NormalizedLocations { + locations, + truncated, + } = self .handle_goto::( &file_path, position, @@ -515,7 +569,10 @@ impl Translator { ) .await?; - Ok(LocationsResult { locations }) + Ok(LocationsResult { + locations, + truncated, + }) } /// Handle go-to-type-definition request (`textDocument/typeDefinition`). @@ -534,7 +591,10 @@ impl Translator { file_path: String, position: Position, ) -> Result { - let locations = self + let NormalizedLocations { + locations, + truncated, + } = self .handle_goto::( &file_path, position, @@ -543,7 +603,10 @@ impl Translator { ) .await?; - Ok(LocationsResult { locations }) + Ok(LocationsResult { + locations, + truncated, + }) } } @@ -561,8 +624,9 @@ mod tests { use url::Url; use super::*; - use crate::bridge::NotificationCache; + use crate::bridge::encoding::PositionEncoding; use crate::bridge::translator::testing::*; + use crate::bridge::{NotificationCache, lock_std, path_to_uri}; use crate::config::ServerId; // ----------------------------------------------------------------- @@ -1380,6 +1444,69 @@ mod tests { ); } + /// Regression for #474/M4: `get_references`' tool description no longer + /// promises "all" references, since a response past + /// `MAX_NORMALIZED_LOCATIONS` is capped -- the client must be able to + /// detect that via `ReferencesResult::truncated` rather than silently + /// receiving a partial result that looks complete. + #[tokio::test] + async fn test_handle_references_sets_truncated_flag_past_cap() { + let dir = TempDir::new().unwrap(); + let server_id = ServerId::from("rust"); + let caps = lsp_types::ServerCapabilities { + references_provider: Some(lsp_types::ReferencesProvider::Bool(true)), + ..Default::default() + }; + let (translator, mut server) = translator_with_capabilities(&dir, &server_id, caps); + + let path = dir.path().join("main.rs"); + fs::write(&path, "fn main() {}").unwrap(); + let uri = Url::from_file_path(&path).unwrap().to_string(); + + let translator = Arc::new(translator); + let handle = { + let translator = Arc::clone(&translator); + let path = path.to_string_lossy().to_string(); + tokio::spawn(async move { translator.handle_references(path, pos(1, 1), true).await }) + }; + + let mut wire = BufReader::new(&mut server.write_stdout); + let opened = read_framed_message(&mut wire).await; + assert_eq!(opened["method"], "textDocument/didOpen"); + let request = read_framed_message(&mut wire).await; + assert_eq!(request["method"], "textDocument/references"); + + let locations: Vec = (0..MAX_NORMALIZED_LOCATIONS + 500) + .map(|_| { + serde_json::json!({ + "uri": uri, + "range": { + "start": {"line": 0, "character": 0}, + "end": {"line": 0, "character": 4} + } + }) + }) + .collect(); + write_response( + &mut server.read_half_stdin, + &request["id"], + serde_json::json!(locations), + ) + .await; + + let result = timeout(Duration::from_secs(5), handle) + .await + .expect("handle_references should not hang") + .unwrap() + .unwrap(); + + assert_eq!(result.locations.len(), MAX_NORMALIZED_LOCATIONS); + assert!( + result.truncated, + "a references response past MAX_NORMALIZED_LOCATIONS must set truncated: true" + ); + } + /// Success-path coverage for `handle_implementation` through the /// `Definition::LocationList` -> `GotoKind::Definition` arm, pinning the /// `GotoResponse` impl for `ImplementationResponse`. @@ -1529,4 +1656,95 @@ mod tests { assert_eq!(result.locations[0].uri, target_uri); assert_eq!(result.locations[0].range.start.character, 8); } + + // ----------------------------------------------------------------- + // Resource-amplification defenses (#474) + // ----------------------------------------------------------------- + + /// Regression for #474's exact attack scenario: many `Location`s + /// clustered onto a handful of distinct `(file, line)` pairs must cost + /// one disk read per distinct pair, not one per location. Proven through + /// the real `lsp_locations_to_mcp` entry point shared by `handle_goto` + /// and `handle_references` -- not by calling the cache-backed helper + /// directly -- and via the cache's own size, which is a direct count of + /// how many times the disk-read fallback actually ran. + #[tokio::test] + async fn test_lsp_locations_to_mcp_reads_disk_once_per_distinct_file_line() { + let dir = TempDir::new().unwrap(); + let mut uris = Vec::new(); + for i in 0..3 { + let path = dir.path().join(format!("file{i}.rs")); + fs::write(&path, "hello").unwrap(); + uris.push(path_to_uri(&path).unwrap()); + } + + let ctx = test_ctx_with(PositionEncoding::Utf8); + // 300 locations, but only 3 distinct (file, line) pairs. + let locs: Vec = (0..300) + .map(|i| lsp_types::Location { + uri: uris[i % 3].clone(), + range: lsp_types::Range { + start: lsp_types::Position { + line: 0, + character: 0, + }, + end: lsp_types::Position { + line: 0, + character: 3, + }, + }, + }) + .collect(); + + let result = lsp_locations_to_mcp(locs, &ctx).await; + + assert_eq!(result.locations.len(), 300); + assert!(!result.truncated); + assert!( + result.locations.iter().all(|l| l.range.end.character == 4), + "MCP columns are 1-based, so LSP byte offset 3 in all-ASCII \"hello\" must convert \ + to 4" + ); + assert_eq!( + lock_std(&ctx.line_cache).entries.len(), + 3, + "300 locations across 3 distinct files must populate the cache with exactly 3 \ + entries (one disk read per distinct file/line), not one per location" + ); + } + + /// Regression for #474: without a cap, a response naming an unbounded + /// number of locations would drive an unbounded number of range + /// conversions. `Utf16` needs no disk read at all (see `test_ctx`), so + /// this isolates the truncation itself from I/O cost -- a response well + /// past `MAX_NORMALIZED_LOCATIONS` must be truncated to it, not hang, + /// OOM, or panic. + #[tokio::test] + async fn test_lsp_locations_to_mcp_truncates_to_max_normalized_locations() { + let ctx = test_ctx(); + let uri = test_uri(); + let locs: Vec = (0..MAX_NORMALIZED_LOCATIONS + 500) + .map(|_| lsp_types::Location { + uri: uri.clone(), + range: lsp_types::Range { + start: lsp_types::Position { + line: 0, + character: 0, + }, + end: lsp_types::Position { + line: 0, + character: 1, + }, + }, + }) + .collect(); + + let result = lsp_locations_to_mcp(locs, &ctx).await; + + assert_eq!(result.locations.len(), MAX_NORMALIZED_LOCATIONS); + assert!( + result.truncated, + "a response naming more than MAX_NORMALIZED_LOCATIONS must report truncated: true" + ); + } } diff --git a/crates/mcpls-core/src/bridge/translator/symbols.rs b/crates/mcpls-core/src/bridge/translator/symbols.rs index 7ab77a86..b41e7fe1 100644 --- a/crates/mcpls-core/src/bridge/translator/symbols.rs +++ b/crates/mcpls-core/src/bridge/translator/symbols.rs @@ -11,6 +11,7 @@ use super::dto::{ lsp_kind_to_u32, }; use super::encoding_ctx::EncodingCtx; +use super::navigation::MAX_NORMALIZED_LOCATIONS; use super::routing::{Capability, IndexingGate}; use crate::bridge::lock_std; use crate::config::{NoServerReason, ToolKind}; @@ -242,7 +243,13 @@ impl Translator { .await?; let ctx = self.encoding_ctx(&server_id); - let mut symbols: Vec = Vec::new(); + + // Collected without normalizing each symbol's range yet, so + // `kind_filter`/`limit` below can drop entries before paying for + // `EncodingCtx::normalize_range` (a disk read on a cache miss) on + // each one -- bounds the *normalization* loop's cost to `limit`; + // this collection loop itself still scans the full response (#474). + let mut raw_symbols: Vec = Vec::new(); match response { // Not filtered to workspace roots -- like other read-only // navigation results (see `uri_in_workspace_roots`'s doc @@ -252,29 +259,20 @@ impl Translator { // `validate_path_against_roots` gate. Some(lsp_types::WorkspaceSymbolResponse::SymbolInformationList(list)) => { for sym in list { - let range = ctx - .normalize_range(&sym.location.uri, sym.location.range) - .await; - symbols.push(WorkspaceSymbol { + raw_symbols.push(RawWorkspaceSymbol { name: sym.base_symbol_information.name, kind: lsp_kind_to_u32(sym.base_symbol_information.kind), - location: Location { - uri: sym.location.uri.to_string(), - range, - out_of_workspace: ctx.is_out_of_workspace(&sym.location.uri), - }, container_name: sym.base_symbol_information.container_name, + out_of_workspace: ctx.is_out_of_workspace(&sym.location.uri), + uri: sym.location.uri, + range: sym.location.range, }); } } Some(lsp_types::WorkspaceSymbolResponse::WorkspaceSymbolList(list)) => { for sym in list { - let (uri, out_of_workspace, range) = match sym.location { - lsp_types::WorkspaceSymbolLocation::Location(loc) => { - let out_of_workspace = ctx.is_out_of_workspace(&loc.uri); - let range = ctx.normalize_range(&loc.uri, loc.range).await; - (loc.uri.to_string(), out_of_workspace, range) - } + let (uri, range) = match sym.location { + lsp_types::WorkspaceSymbolLocation::Location(loc) => (loc.uri, loc.range), // `LocationUriOnly` carries no range -- the server // deliberately withheld it (e.g. to avoid computing it // eagerly for every workspace-search result). The MCP @@ -284,33 +282,60 @@ impl Translator { // symbol is dropped rather than inventing coordinates. lsp_types::WorkspaceSymbolLocation::LocationUriOnly(_) => continue, }; - symbols.push(WorkspaceSymbol { + raw_symbols.push(RawWorkspaceSymbol { name: sym.base_symbol_information.name, kind: lsp_kind_to_u32(sym.base_symbol_information.kind), - location: Location { - uri, - range, - out_of_workspace, - }, container_name: sym.base_symbol_information.container_name, + out_of_workspace: ctx.is_out_of_workspace(&uri), + uri, + range, }); } } None => {} } - // Apply kind filter if specified. if let Some(target) = kind_filter { - symbols.retain(|s| s.kind == target); + raw_symbols.retain(|s| s.kind == target); + } + // `limit` is clamped to MAX_NORMALIZED_LOCATIONS (else u32::MAX + // would reopen the unbounded normalization loop, see #474). + let effective_limit = (limit as usize).min(MAX_NORMALIZED_LOCATIONS); + let truncated = raw_symbols.len() > effective_limit; + raw_symbols.truncate(effective_limit); + + let mut symbols = Vec::with_capacity(raw_symbols.len()); + for raw in raw_symbols { + let range = ctx.normalize_range(&raw.uri, raw.range).await; + symbols.push(WorkspaceSymbol { + name: raw.name, + kind: raw.kind, + location: Location { + uri: raw.uri.to_string(), + range, + out_of_workspace: raw.out_of_workspace, + }, + container_name: raw.container_name, + }); } - // Limit results - symbols.truncate(limit as usize); - - Ok(WorkspaceSymbolResult { symbols }) + Ok(WorkspaceSymbolResult { symbols, truncated }) } } +/// A workspace symbol not yet normalized into MCP coordinates -- lets +/// [`Translator::handle_workspace_symbol`] apply `kind_filter`/`limit` before +/// paying for [`EncodingCtx::normalize_range`] on each surviving entry (see +/// #474). +struct RawWorkspaceSymbol { + name: String, + kind: u32, + container_name: Option, + out_of_workspace: bool, + uri: lsp_types::Uri, + range: lsp_types::Range, +} + #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { @@ -776,6 +801,76 @@ mod tests { ); } + /// Regression for #474: `limit` is a caller-supplied `u32` with no + /// upper bound of its own -- `limit: u32::MAX` must still be clamped to + /// `MAX_NORMALIZED_LOCATIONS`, not restore the unbounded normalization + /// loop the cap exists to prevent. + #[tokio::test] + async fn test_handle_workspace_symbol_clamps_limit_to_max_normalized_locations() { + let dir = TempDir::new().unwrap(); + let server_id = ServerId::from("rust"); + let caps = lsp_types::ServerCapabilities { + workspace_symbol_provider: Some(lsp_types::WorkspaceSymbolProvider::Bool(true)), + ..Default::default() + }; + let (translator, mut server) = translator_with_capabilities(&dir, &server_id, caps); + let uri = Url::from_file_path(dir.path().join("many.rs")) + .unwrap() + .to_string(); + + let translator = Arc::new(translator); + let handle = { + let translator = Arc::clone(&translator); + tokio::spawn(async move { + translator + .handle_workspace_symbol("foo".to_string(), None, u32::MAX) + .await + }) + }; + + let mut wire = BufReader::new(&mut server.write_stdout); + let request = read_framed_message(&mut wire).await; + assert_eq!(request["method"], "workspace/symbol"); + + let symbols: Vec = (0..MAX_NORMALIZED_LOCATIONS + 500) + .map(|i| { + serde_json::json!({ + "name": format!("sym{i}"), + "kind": 12, + "location": { + "uri": uri, + "range": { + "start": {"line": 0, "character": 0}, + "end": {"line": 0, "character": 3} + } + } + }) + }) + .collect(); + write_response( + &mut server.read_half_stdin, + &request["id"], + serde_json::json!(symbols), + ) + .await; + + let result = timeout(Duration::from_secs(5), handle) + .await + .expect("handler call should not hang") + .unwrap() + .unwrap(); + + assert_eq!( + result.symbols.len(), + MAX_NORMALIZED_LOCATIONS, + "limit: u32::MAX must be clamped to MAX_NORMALIZED_LOCATIONS, not left unbounded" + ); + assert!( + result.truncated, + "a limit clamped below what the caller asked for must set truncated: true" + ); + } + /// `document_symbols` is single-file analysis, valid even mid-index /// (spec FR-008), so `IndexingGate::NotRequired` at its /// `prepare_gated_document` call site must mean it dispatches even diff --git a/crates/mcpls-core/src/bridge/translator/testing.rs b/crates/mcpls-core/src/bridge/translator/testing.rs index e3178528..568440e4 100644 --- a/crates/mcpls-core/src/bridge/translator/testing.rs +++ b/crates/mcpls-core/src/bridge/translator/testing.rs @@ -10,7 +10,7 @@ use tempfile::TempDir; use super::Translator; use super::dto::Position; -use super::encoding_ctx::EncodingCtx; +use super::encoding_ctx::{EncodingCtx, new_line_cache}; use crate::bridge::encoding::PositionEncoding; use crate::bridge::state::ResourceLimits; use crate::bridge::{DiagnosticInfo, DocumentTracker}; @@ -51,6 +51,7 @@ pub(super) fn test_ctx_with_roots( HashMap::new(), )), workspace_roots: Arc::new(workspace_roots), + line_cache: new_line_cache(), } } diff --git a/crates/mcpls-core/src/mcp/server.rs b/crates/mcpls-core/src/mcp/server.rs index a373a616..148a9d6d 100644 --- a/crates/mcpls-core/src/mcp/server.rs +++ b/crates/mcpls-core/src/mcp/server.rs @@ -538,7 +538,7 @@ impl McplsServer { /// Get the definition location of a symbol. #[tool( - description = "Definition location of symbol at position. Returns file path, line, and character where declared.", + description = "Definition location of symbol at position. Returns file path, line, and character where declared. Capped at a fixed maximum for a pathological case; `truncated: true` on the result means more locations exist than are returned.", title = "Go to Definition" )] async fn get_definition( @@ -559,7 +559,7 @@ impl McplsServer { /// Find all references to a symbol. #[tool( - description = "All references to symbol at position. Returns locations across workspace where symbol is used.", + description = "References to symbol at position, across workspace. Capped at a fixed maximum for an extremely common symbol; `truncated: true` on the result means more references exist than are returned.", title = "Find References" )] async fn get_references( @@ -737,7 +737,7 @@ impl McplsServer { /// Search for symbols across the workspace. #[tool( - description = "Search workspace symbols by name. Supports partial matching and fuzzy search.", + description = "Search workspace symbols by name. Supports partial matching and fuzzy search. `limit` is capped at a fixed server-side maximum regardless of the value requested; `truncated: true` on the result means more matches exist than are returned.", title = "Workspace Symbol Search" )] async fn workspace_symbol_search( @@ -958,7 +958,7 @@ impl McplsServer { /// Go to implementation locations. #[tool( - description = "Implementation locations of trait method or interface member at position.", + description = "Implementation locations of trait method or interface member at position. Capped at a fixed maximum for an extremely common trait/interface; `truncated: true` on the result means more implementations exist than are returned.", title = "Go to Implementation" )] async fn go_to_implementation( @@ -979,7 +979,7 @@ impl McplsServer { /// Go to type definition location. #[tool( - description = "Type definition location of expression at position. Distinct from go-to-definition for variable bindings.", + description = "Type definition location of expression at position. Distinct from go-to-definition for variable bindings. Capped at a fixed maximum for a pathological case; `truncated: true` on the result means more locations exist than are returned.", title = "Go to Type Definition" )] async fn go_to_type_definition( diff --git a/crates/mcpls-core/src/mcp/tool_surface.json b/crates/mcpls-core/src/mcp/tool_surface.json index cdcd3755..407936fc 100644 --- a/crates/mcpls-core/src/mcp/tool_surface.json +++ b/crates/mcpls-core/src/mcp/tool_surface.json @@ -166,7 +166,7 @@ { "name": "get_definition", "title": "Go to Definition", - "description": "Definition location of symbol at position. Returns file path, line, and character where declared.", + "description": "Definition location of symbol at position. Returns file path, line, and character where declared. Capped at a fixed maximum for a pathological case; `truncated: true` on the result means more locations exist than are returned.", "inputSchema": { "$schema": "https://json-schema.org/draft/2020-12/schema", "properties": { @@ -267,6 +267,10 @@ "$ref": "#/$defs/Location" }, "type": "array" + }, + "truncated": { + "description": "Whether `locations` was capped below the LSP server's full response\n(see `MAX_NORMALIZED_LOCATIONS`, #474) -- if `true`, more locations\nexist than are returned here. Omitted (defaults to `false`) when\nserialized.", + "type": "boolean" } }, "required": [ @@ -684,7 +688,7 @@ { "name": "get_references", "title": "Find References", - "description": "All references to symbol at position. Returns locations across workspace where symbol is used.", + "description": "References to symbol at position, across workspace. Capped at a fixed maximum for an extremely common symbol; `truncated: true` on the result means more references exist than are returned.", "inputSchema": { "$schema": "https://json-schema.org/draft/2020-12/schema", "properties": { @@ -790,6 +794,10 @@ "$ref": "#/$defs/Location" }, "type": "array" + }, + "truncated": { + "description": "Whether `locations` was capped below the LSP server's full response\n(see `MAX_NORMALIZED_LOCATIONS`, #474) -- if `true`, more references\nexist than are returned here. Omitted (defaults to `false`) when\nserialized.", + "type": "boolean" } }, "required": [ @@ -900,7 +908,7 @@ { "name": "go_to_implementation", "title": "Go to Implementation", - "description": "Implementation locations of trait method or interface member at position.", + "description": "Implementation locations of trait method or interface member at position. Capped at a fixed maximum for an extremely common trait/interface; `truncated: true` on the result means more implementations exist than are returned.", "inputSchema": { "$schema": "https://json-schema.org/draft/2020-12/schema", "properties": { @@ -938,7 +946,7 @@ { "name": "go_to_type_definition", "title": "Go to Type Definition", - "description": "Type definition location of expression at position. Distinct from go-to-definition for variable bindings.", + "description": "Type definition location of expression at position. Distinct from go-to-definition for variable bindings. Capped at a fixed maximum for a pathological case; `truncated: true` on the result means more locations exist than are returned.", "inputSchema": { "$schema": "https://json-schema.org/draft/2020-12/schema", "properties": { @@ -1057,7 +1065,7 @@ { "name": "workspace_symbol_search", "title": "Workspace Symbol Search", - "description": "Search workspace symbols by name. Supports partial matching and fuzzy search.", + "description": "Search workspace symbols by name. Supports partial matching and fuzzy search. `limit` is capped at a fixed server-side maximum regardless of the value requested; `truncated: true` on the result means more matches exist than are returned.", "inputSchema": { "$schema": "https://json-schema.org/draft/2020-12/schema", "properties": { From 982386437cf58fe0af9e51228733f3393156dabd Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 21 Sep 2026 14:02:07 +0200 Subject: [PATCH 2/2] docs(changelog): link #474 entry to its PR --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ea06e2b..24b53d52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,7 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - LSP frame header parsing is now bounded on both single-line length and header count per frame, closing an unbounded-memory-growth path from a malicious or malfunctioning spawned LSP server. (#457, #463) - CodeQL workflow now triggers on `pull_request` instead of `pull_request_target`, closing a "pwn request" path where a fork PR's `build.rs`/proc-macros ran during `autobuild` with a base-repository `GITHUB_TOKEN` and could poison the default-branch Actions cache. (#464, #469) - **Workspace-roots validation now fails closed** — Breaking change: the diagnostics pump and rename/code-action edit filtering previously allowed unrestricted access when no workspace roots were configured; both now reject with no unrestricted opt-in, so embedders must call `Translator::set_workspace_roots` with real roots before serving any path-taking request. (#449) -- Position-encoding conversion no longer amplifies one LSP response into unbounded disk reads: line text is now cached and read incrementally per response, and a per-response byte budget bounds the disk I/O any navigation/references/workspace-symbol/call-hierarchy/inlay-hints/rename response can trigger; references, goto-X, and workspace-symbol are additionally capped on result count. (#474) +- Position-encoding conversion no longer amplifies one LSP response into unbounded disk reads: line text is now cached and read incrementally per response, and a per-response byte budget bounds the disk I/O any navigation/references/workspace-symbol/call-hierarchy/inlay-hints/rename response can trigger; references, goto-X, and workspace-symbol are additionally capped on result count. (#474, #486) ## [0.5.0] - 2026-09-06