diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c9ca5e7..e00c7024 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **`--trust-project-config` flag / `MCPLS_TRUST_PROJECT_CONFIG` env var** — opt-in gate for loading a `./mcpls.toml` discovered relative to the current directory. New `config::ProjectConfigTrust` enum and `ServerConfig::load_with_trust(trust)`, which `ServerConfig::load()` now delegates to with `ProjectConfigTrust::Untrusted`. (#229) - **Explicit per-tool routing** — `[[lsp_servers]]` entries gain optional `name` (routing identity, defaults to `language_id`) and `handles` (list of tools this server serves; omitted means catch-all) fields, so two servers can share one `language_id` and each own a distinct subset of MCP tools (e.g. pyright for everything, pylsp for `diagnostics` only). See `docs/user-guide/configuration.md` for the full semantics, including what happens when the routed server fails to spawn. (#174) - **`config::ToolKind` / `config::ServerId` / `config::ToolRouter`** — new public types: `ToolKind` is the typed enum of every routable MCP tool; `ServerId` is a server's routing identity; `ToolRouter` resolves `(language, tool)` to a `ServerId`, enforces the workspace-scoped routing rules at startup, and rebinds dead routes to a live catch-all (never to a server that explicitly declined the tool) once server registration completes. (#174) - **Single source of truth for React language-ID variants** — new `config::react_variant_language_id` / `config::base_language_id`, consumed by both `language_id_for_pattern_extension` and the translator's per-file server routing, replacing two independent hardcoded match arms. (#165) @@ -24,7 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **Translator lock contention on concurrent tool calls** — `Translator` no longer sits behind a single `Arc>` held for the full duration of an LSP round trip. Read-only config (`lsp_clients`, `workspace_roots`, `extension_map`, `expected_languages`) is now `Arc`-shared with independent, short-lived locks, and the document tracker has its own lock acquired only for `ensure_open`. The actual LSP request/response round trip now runs with no lock held at all, so a slow in-flight call (e.g. `get_diagnostics` pulling diagnostics from a busy language server) no longer stalls unrelated calls like `get_hover` behind it for up to 30s. The `ensure_open` prepare step still serializes through one shared document-tracker lock (bounded by a 250ms disk-check debounce in the common case; see `TODO(critic-S2)` in `translator.rs` for the follow-up to scope it per path). (#108, #159) +- **Untrusted project-local config could redirect the spawned LSP command (P1 security)** — `ServerConfig::load()` discovered and loaded `./mcpls.toml` relative to the process's current working directory unconditionally, and that file's `command`/`args` (and `[workspace]`, e.g. `roots`, `heuristics_max_depth`) fed directly into the LSP server mcpls spawns. Running `mcpls` against an untrusted checkout (e.g. `git clone && mcpls`) could execute an attacker-chosen command with no confirmation — not viable to gate interactively since stdio *is* the MCP transport. Breaking change (acceptable pre-1.0): a CWD-discovered `./mcpls.toml` is now ignored by default (a warning names the ignored path); pass `--trust-project-config` or set `MCPLS_TRUST_PROJECT_CONFIG=true` to opt in. An explicit `--config`/`MCPLS_CONFIG` path is unaffected — naming a path is itself consent. Built-in project-marker heuristics (e.g. `Cargo.toml` → rust-analyzer) still apply normally when the project-local file is ignored. Note: the original report's claim that the config's `env` field is attacker-controlled at spawn time does not hold today — `env` is parsed but never passed to the spawned process (no `.envs()` call exists), so this fix is scoped to `command`/`args`/`[workspace]`. (#229) +- **Translator lock contention on concurrent tool calls** — `Translator` no longer sits behind a single `Arc>` held for the full duration of an LSP round trip. Read-only config (`lsp_clients`, `workspace_roots`, `extension_map`, `expected_languages`) is now `Arc`-shared with independent, short-lived locks, and the document tracker has its own lock acquired only for `ensure_open`. The actual LSP request/response round trip now runs with no lock held at all, so a slow in-flight call (e.g. `get_diagnostics` pulling diagnostics from a busy language server) no longer stalls unrelated calls like `get_hover` behind it for up to 30s. (#108, #159) +- **`document_tracker` lock still spanned disk I/O and the `didOpen`/`didChange` notify in `ensure_open` (P1)** — the lock fixed above was a single lock shared across every language and path, held across `ensure_open`'s own awaits (a `stat`, optionally a full re-read of the file, and the LSP notify — a bounded channel send with no timeout). A wedged language server that stopped draining its stdin could stall `ensure_open` for every other file and language, not just the one talking to that server. `DocumentTracker` now locks its document map only for short, synchronous sections and serializes `ensure_open` per path via a keyed async lock, so calls for different paths never wait on each other; calls for the same path still collapse into exactly one `didOpen`. Breaking change (acceptable pre-1.0): every `DocumentTracker` method now takes `&self` instead of `&mut self`; `get` returns an owned `DocumentState` instead of `Option<&DocumentState>`; `open_paths` returns `Vec` instead of an iterator; `Translator::document_tracker` is now a plain `Arc` instead of `Arc>`, and `Translator::open_document_paths`/`is_document_open` are no longer `async`. (#227) - **Stale document tracker on external file changes** — `DocumentTracker::ensure_open` now stats the file on every call and resyncs the LSP server via a single full-replacement `textDocument/didChange` when the on-disk content changed. Previously a file was only ever read once per session, so external edits (git checkout/stash, formatters, the MCP host's own Edit/Write tools) went unnoticed by mcpls and produced stale hover/diagnostics/completion results until the process restarted. (#102) - **CodeQL fork PR checkout** — `actions/checkout@v7` refuses to check out a fork PR's head SHA in `pull_request_target` workflows unless explicitly opted in; set `allow-unsafe-pr-checkout: true` on the CodeQL workflow's checkout step, which was failing every fork-originated PR since the checkout v6 → v7 bump. Safe here because the job carries no secrets and permissions are scoped to `security-events: write` + `contents: read` only. - **Notification loss and cached-read stalls under translator-lock contention** — `NotificationCache` is now held behind its own `Arc>`, independent of `Arc>`, and workspace-root path validation for cache-only reads uses a lock-free `Arc<[PathBuf]>` snapshot instead of locking the translator. Previously, `diagnostics_pump` wrote into the cache through the translator lock: while that lock was held elsewhere for the duration of a slow `textDocument/diagnostic` round-trip, incoming `publishDiagnostics`/log/message notifications were silently dropped (the LSP transport forwards them via a non-blocking `try_send`), and `get_cached_diagnostics`/`read_resource`/`subscribe` — which only ever needed a cached read or a path check — queued behind that same round-trip for as long as it took to complete. (#104) diff --git a/README.md b/README.md index a825c520..76c44945 100644 --- a/README.md +++ b/README.md @@ -212,6 +212,7 @@ project_markers = ["Cargo.toml", "rust-toolchain.toml", ".rust-version"] | Variable | Description | Default | |----------|-------------|---------| | `MCPLS_CONFIG` | Path to configuration file | Auto-detected | +| `MCPLS_TRUST_PROJECT_CONFIG` | Load a `./mcpls.toml` found in the current directory | `false` | | `MCPLS_LOG` | Log level (trace, debug, info, warn, error) | `info` | | `MCPLS_LOG_JSON` | Output logs as JSON | `false` | @@ -223,6 +224,13 @@ project_markers = ["Cargo.toml", "rust-toolchain.toml", ".rust-version"] | macOS | `~/.config/mcpls/mcpls.toml` or `~/Library/Application Support/mcpls/` | | Windows | `%APPDATA%\mcpls\mcpls.toml` | +> [!WARNING] +> A `./mcpls.toml` in the current directory is **not** loaded automatically: it +> can control which command mcpls spawns as an LSP server, so running mcpls +> against an untrusted checkout must not execute commands from that checkout +> without explicit consent. Pass `--trust-project-config` (or set +> `MCPLS_TRUST_PROJECT_CONFIG=true`) only for repositories you trust. +
diff --git a/crates/mcpls-cli/README.md b/crates/mcpls-cli/README.md index 1bc23985..c3f61bfb 100644 --- a/crates/mcpls-cli/README.md +++ b/crates/mcpls-cli/README.md @@ -28,9 +28,15 @@ mcpls --listen 127.0.0.1:3000 # HTTP transport (transport-http featu ## Configuration > [!NOTE] -> Configuration auto-discovery order: `$MCPLS_CONFIG` → `./mcpls.toml` → platform config dir +> Configuration auto-discovery order: `$MCPLS_CONFIG` → `./mcpls.toml` (requires +> `--trust-project-config`) → platform config dir > Auto-creates default config with 30 language mappings on first run. +> [!WARNING] +> A `./mcpls.toml` in the current directory is ignored by default, since it can +> control which command mcpls spawns as an LSP server. Pass `--trust-project-config` +> (or set `MCPLS_TRUST_PROJECT_CONFIG=true`) only for repositories you trust. + Create or edit `mcpls.toml` in the appropriate location: - **Linux/macOS:** `~/.config/mcpls/mcpls.toml` - **macOS (alternative):** `~/Library/Application Support/mcpls/mcpls.toml` @@ -43,6 +49,7 @@ See the main [README](../../README.md) for configuration examples and custom ext | Flag | Env | Description | |------|-----|-------------| | `-c, --config ` | `MCPLS_CONFIG` | Configuration file path | +| `--trust-project-config` | `MCPLS_TRUST_PROJECT_CONFIG` | Load a `./mcpls.toml` found in the current directory | | `-l, --log-level ` | `MCPLS_LOG` | trace, debug, info, warn, error (default: info) | | `--log-json` | `MCPLS_LOG_JSON` | JSON-formatted logs for tooling | | `--listen ` | `MCPLS_LISTEN` | Bind address for HTTP transport (`transport-http` feature) | diff --git a/crates/mcpls-cli/src/args.rs b/crates/mcpls-cli/src/args.rs index 761582e4..c1441abf 100644 --- a/crates/mcpls-cli/src/args.rs +++ b/crates/mcpls-cli/src/args.rs @@ -17,11 +17,23 @@ pub struct Args { /// /// If not specified, searches for mcpls.toml in: /// 1. `$MCPLS_CONFIG` environment variable - /// 2. Current directory + /// 2. Current directory (only loaded with `--trust-project-config`) /// 3. ~/.config/mcpls/mcpls.toml #[arg(short, long, value_name = "FILE", env = "MCPLS_CONFIG")] pub config: Option, + /// Trust and load a `mcpls.toml` found in the current directory. + /// + /// A project-local config discovered this way (as opposed to one passed + /// explicitly via `--config`/`MCPLS_CONFIG`) can control the LSP server + /// `command`/`args` mcpls spawns, so it is ignored by default to avoid + /// arbitrary code execution when running mcpls against an untrusted + /// checkout. Pass this flag only for repositories you trust. Via + /// `MCPLS_TRUST_PROJECT_CONFIG`, only the literal values `true`/`false` + /// are accepted; any other value is a parse error at startup. + #[arg(long, env = "MCPLS_TRUST_PROJECT_CONFIG")] + pub trust_project_config: bool, + /// Logging level /// /// Valid values: trace, debug, info, warn, error @@ -65,6 +77,18 @@ mod tests { assert!(!args.log_json); } + #[test] + fn test_trust_project_config_default_false() { + let args = Args::parse_from(["mcpls"]); + assert!(!args.trust_project_config); + } + + #[test] + fn test_trust_project_config_flag() { + let args = Args::parse_from(["mcpls", "--trust-project-config"]); + assert!(args.trust_project_config); + } + #[test] fn test_config_arg() { let args = Args::parse_from(["mcpls", "--config", "/path/to/config.toml"]); diff --git a/crates/mcpls-cli/src/main.rs b/crates/mcpls-cli/src/main.rs index 976b9700..a8234dcc 100644 --- a/crates/mcpls-cli/src/main.rs +++ b/crates/mcpls-cli/src/main.rs @@ -5,6 +5,7 @@ use anyhow::{Context, Result}; use clap::Parser; +use mcpls_core::ProjectConfigTrust; mod args; mod logging; @@ -25,7 +26,12 @@ async fn main() -> Result<()> { mcpls_core::ServerConfig::load_from(config_path) .with_context(|| format!("failed to load config from {}", config_path.display()))? } else { - mcpls_core::ServerConfig::load().context("failed to load configuration")? + let trust = if args.trust_project_config { + ProjectConfigTrust::Trusted + } else { + ProjectConfigTrust::Untrusted + }; + mcpls_core::ServerConfig::load_with_trust(trust).context("failed to load configuration")? }; tracing::debug!( diff --git a/crates/mcpls-cli/tests/cli_integration.rs b/crates/mcpls-cli/tests/cli_integration.rs index 333585dc..b21ff024 100644 --- a/crates/mcpls-cli/tests/cli_integration.rs +++ b/crates/mcpls-cli/tests/cli_integration.rs @@ -5,16 +5,32 @@ use std::fs; use std::process::Command; +use std::time::Duration; use assert_cmd::prelude::*; use predicates::prelude::*; use tempfile::TempDir; +/// Vars that could leak in from the ambient environment (e.g. a developer's +/// shell, or a repo `.envrc`) and change these tests' outcome: `MCPLS_LOG` +/// could suppress a warning a test greps for, `MCPLS_CONFIG` could redirect +/// config loading away from the CWD/`--config` path under test, and +/// `MCPLS_TRUST_PROJECT_CONFIG` could flip the trust decision a test is +/// specifically exercising. `assert_cmd::Command`/`std::process::Command` +/// inherit the parent's full environment by default, so every test must +/// clear these before setting the ones it actually wants. +fn clear_ambient_env(cmd: &mut Command) -> &mut Command { + cmd.env_remove("MCPLS_LOG") + .env_remove("MCPLS_CONFIG") + .env_remove("MCPLS_TRUST_PROJECT_CONFIG") +} + #[test] fn test_help_flag() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("--help") + clear_ambient_env(&mut cmd) + .arg("--help") .assert() .success() .stdout(predicate::str::contains("--config")); @@ -24,7 +40,8 @@ fn test_help_flag() { fn test_version_flag() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("--version") + clear_ambient_env(&mut cmd) + .arg("--version") .assert() .success() .stdout(predicate::str::contains(env!("CARGO_PKG_VERSION"))); @@ -34,7 +51,8 @@ fn test_version_flag() { fn test_version_short_flag() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("-V") + clear_ambient_env(&mut cmd) + .arg("-V") .assert() .success() .stdout(predicate::str::contains(env!("CARGO_PKG_VERSION"))); @@ -44,7 +62,8 @@ fn test_version_short_flag() { fn test_help_short_flag() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("-h") + clear_ambient_env(&mut cmd) + .arg("-h") .assert() .success() .stdout(predicate::str::contains("--config")); @@ -54,7 +73,8 @@ fn test_help_short_flag() { fn test_invalid_flag() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("--invalid-flag") + clear_ambient_env(&mut cmd) + .arg("--invalid-flag") .assert() .failure() .stderr(predicate::str::contains("unexpected argument")); @@ -64,7 +84,8 @@ fn test_invalid_flag() { fn test_config_file_not_found() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("--config") + clear_ambient_env(&mut cmd) + .arg("--config") .arg("/nonexistent/path/to/config.toml") .assert() .failure() @@ -80,7 +101,8 @@ fn test_config_with_invalid_toml() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("--config") + clear_ambient_env(&mut cmd) + .arg("--config") .arg(&config_path) .assert() .failure() @@ -91,7 +113,8 @@ fn test_config_with_invalid_toml() { fn test_config_short_flag() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("-c") + clear_ambient_env(&mut cmd) + .arg("-c") .arg("/nonexistent/config.toml") .assert() .failure() @@ -107,7 +130,135 @@ fn test_config_with_empty_file() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("--config").arg(&config_path).assert().failure(); + clear_ambient_env(&mut cmd) + .arg("--config") + .arg(&config_path) + .assert() + .failure(); +} + +/// A CWD-discovered `./mcpls.toml` is untrusted by default: it must not be +/// parsed at all, regardless of `--config`/`MCPLS_CONFIG` (which are +/// unaffected by trust and aren't exercised here). We assert this by +/// planting an invalid TOML file and confirming the process does *not* fail +/// with a config-parse error — instead it logs the ignore-warning and +/// proceeds to serve (which blocks on stdio, so it's killed once the +/// timeout elapses; the kill itself is expected, not a test failure). +#[test] +fn test_trust_project_config_env_false_does_not_grant_trust() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("mcpls.toml"); + fs::write(&config_path, "this is not valid TOML {{{{").unwrap(); + + let mut cmd = assert_cmd::Command::cargo_bin("mcpls").unwrap(); + cmd.env_remove("MCPLS_LOG") + .env_remove("MCPLS_CONFIG") + .env_remove("MCPLS_TRUST_PROJECT_CONFIG"); + let output = cmd + .current_dir(temp_dir.path()) + .env("MCPLS_TRUST_PROJECT_CONFIG", "false") + // Generous bound: the untrusted path is expected to block on stdio + // (proving it never bailed out on the broken TOML), so this timeout + // always elapses by design. It only needs to be long enough that a + // loaded CI runner doesn't get the process killed before mcpls even + // finishes logging the ignore-warning, which would false-fail this + // test rather than exercise the actual behavior under test. + .timeout(Duration::from_secs(5)) + .output() + .unwrap(); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("ignoring untrusted project-local config"), + "expected the untrusted-ignore warning, got stderr: {stderr}" + ); + assert!( + !stderr.contains("failed to load configuration"), + "MCPLS_TRUST_PROJECT_CONFIG=false must not grant trust; stderr: {stderr}" + ); +} + +/// clap's derived value parser for this flag only accepts the literal +/// `true`/`false` (see `ArgAction::SetTrue`'s `default_value_parser` in +/// `clap_builder`, which uses `ValueParser::bool()`, not a boolish parser +/// accepting `0`/`1`/`yes`/`no`). A malformed value like `0` must be +/// *rejected* outright rather than silently coerced to either trust state — +/// this is the strongest form of "does not grant trust": the process never +/// gets far enough to load anything, trusted or not. We don't pin the exact +/// clap error wording (brittle across clap upgrades); it's enough that the +/// process fails before either trust branch's log line could appear, since +/// argument parsing runs before logging is even initialized. +#[test] +fn test_trust_project_config_env_invalid_value_rejected() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("mcpls.toml"); + fs::write(&config_path, "this is not valid TOML {{{{").unwrap(); + + let mut cmd = Command::cargo_bin("mcpls").unwrap(); + clear_ambient_env(&mut cmd) + .current_dir(temp_dir.path()) + .env("MCPLS_TRUST_PROJECT_CONFIG", "0") + .assert() + .failure() + .stderr(predicate::str::contains("failed to load configuration").not()) + .stderr(predicate::str::contains("ignoring untrusted project-local config").not()); +} + +#[test] +fn test_trust_project_config_flag_grants_trust() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("mcpls.toml"); + fs::write(&config_path, "this is not valid TOML {{{{").unwrap(); + + let mut cmd = Command::cargo_bin("mcpls").unwrap(); + clear_ambient_env(&mut cmd) + .current_dir(temp_dir.path()) + .arg("--trust-project-config") + .assert() + .failure() + .stderr(predicate::str::contains("failed to load configuration")); +} + +#[test] +fn test_trust_project_config_env_true_grants_trust() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("mcpls.toml"); + fs::write(&config_path, "this is not valid TOML {{{{").unwrap(); + + let mut cmd = Command::cargo_bin("mcpls").unwrap(); + clear_ambient_env(&mut cmd) + .current_dir(temp_dir.path()) + .env("MCPLS_TRUST_PROJECT_CONFIG", "true") + .assert() + .failure() + .stderr(predicate::str::contains("failed to load configuration")); +} + +/// `MCPLS_CONFIG` must stay trusted -- and be the file actually consulted -- +/// even when a `./mcpls.toml` also exists in the CWD, regardless of the CWD +/// file's own trust state. Runs with `--trust-project-config` set (so the +/// CWD file is itself trusted) specifically to rule out a reordering bug +/// where a trusted CWD file gets checked/loaded before `MCPLS_CONFIG`: if +/// that ordering regressed, the invalid-TOML CWD file would be parsed first +/// and fail with a TOML-parse error, never reaching the `MCPLS_CONFIG` +/// check. Asserting the "configuration file not found" error for the +/// `MCPLS_CONFIG` path (not a TOML-parse error) proves `MCPLS_CONFIG` is +/// still checked first. +#[test] +fn test_mcpls_config_env_wins_over_cwd_file_even_when_trusted() { + let temp_dir = TempDir::new().unwrap(); + let cwd_config_path = temp_dir.path().join("mcpls.toml"); + fs::write(&cwd_config_path, "this is not valid TOML {{{{").unwrap(); + + let mut cmd = Command::cargo_bin("mcpls").unwrap(); + clear_ambient_env(&mut cmd) + .current_dir(temp_dir.path()) + .arg("--trust-project-config") + .env("MCPLS_CONFIG", "/nonexistent/path/to/config.toml") + .assert() + .failure() + .stderr(predicate::str::contains("configuration file not found")) + .stderr(predicate::str::contains("TOML parsing error").not()); } #[test] @@ -121,5 +272,9 @@ fn test_config_file_with_spaces_in_path() { let mut cmd = Command::cargo_bin("mcpls").unwrap(); - cmd.arg("--config").arg(&config_path).assert().failure(); + clear_ambient_env(&mut cmd) + .arg("--config") + .arg(&config_path) + .assert() + .failure(); } diff --git a/crates/mcpls-core/src/bridge/mod.rs b/crates/mcpls-core/src/bridge/mod.rs index 5acd0e66..acab1692 100644 --- a/crates/mcpls-core/src/bridge/mod.rs +++ b/crates/mcpls-core/src/bridge/mod.rs @@ -3,6 +3,8 @@ //! This module handles the bidirectional conversion between //! MCP tool calls and LSP requests/responses. +use std::sync::{Mutex as StdMutex, MutexGuard, PoisonError}; + mod encoding; mod notifications; pub mod resources; @@ -21,3 +23,16 @@ pub use translator::{ DiagnosticsResult, DocumentChanges, DocumentSymbolsResult, FormatDocumentResult, HoverResult, Location, Position2D, Range, ReferencesResult, RenameResult, Symbol, TextEdit, Translator, }; + +/// Lock a `std::sync::Mutex`, recovering the guard if a previous holder +/// panicked while holding it. +/// +/// Every lock guarded this way protects a short, synchronous, panic-free +/// critical section (a `HashMap`/`HashSet` lookup or insert), so poisoning +/// can only happen if an unrelated bug already panicked; refusing to unwind +/// the whole process a second time over stale poisoning is preferable to +/// deadlocking future calls. Shared by `translator` and `state` so both +/// modules lock their interior `HashMap`/`HashSet` fields the same way. +pub(crate) fn lock_std(mutex: &StdMutex) -> MutexGuard<'_, T> { + mutex.lock().unwrap_or_else(PoisonError::into_inner) +} diff --git a/crates/mcpls-core/src/bridge/state.rs b/crates/mcpls-core/src/bridge/state.rs index e53bd749..a74250f5 100644 --- a/crates/mcpls-core/src/bridge/state.rs +++ b/crates/mcpls-core/src/bridge/state.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex as StdMutex}; use std::time::{Duration, SystemTime}; use lsp_types::{ @@ -12,9 +13,11 @@ use lsp_types::{ }; use tokio::fs; use tokio::io::AsyncReadExt; +use tokio::sync::{Mutex as AsyncMutex, OwnedMutexGuard}; use tokio::time::Instant; use url::Url; +use super::lock_std; use crate::config::ServerId; use crate::error::{Error, Result}; use crate::lsp::LspClient; @@ -147,10 +150,20 @@ impl Default for ResourceLimits { } /// Tracks document state across the workspace. +/// +/// Every method takes `&self`: the document map and the per-path locks used +/// by [`Self::ensure_open`] are both interior-mutable, so a single tracker +/// can be shared behind a plain `Arc` with no outer lock. +/// See [`Self::ensure_open`] for the concurrency contract this maintains. #[derive(Debug)] pub struct DocumentTracker { - /// Open documents by file path. - documents: HashMap, + /// Open documents by file path. Locked only for the short, synchronous + /// section that touches it — never held across an `await`. + documents: StdMutex>, + /// Per-path locks serializing [`Self::ensure_open`] calls for the same + /// path, so calls for different paths never wait on each other. See + /// `lock_path` for how entries are created and evicted. + path_locks: StdMutex>>>, /// Resource limits for tracking. limits: ResourceLimits, /// Custom file extension to language ID mappings. @@ -162,7 +175,8 @@ impl DocumentTracker { #[must_use] pub fn new(limits: ResourceLimits, extension_map: HashMap) -> Self { Self { - documents: HashMap::new(), + documents: StdMutex::new(HashMap::new()), + path_locks: StdMutex::new(HashMap::new()), limits, extension_map, } @@ -171,25 +185,25 @@ impl DocumentTracker { /// Check if a document is currently open. #[must_use] pub fn is_open(&self, path: &Path) -> bool { - self.documents.contains_key(path) + lock_std(&self.documents).contains_key(path) } - /// Get the state of an open document. + /// Get a clone of the state of an open document. #[must_use] - pub fn get(&self, path: &Path) -> Option<&DocumentState> { - self.documents.get(path) + pub fn get(&self, path: &Path) -> Option { + lock_std(&self.documents).get(path).cloned() } /// Get the number of open documents. #[must_use] pub fn len(&self) -> usize { - self.documents.len() + lock_std(&self.documents).len() } /// Check if there are no open documents. #[must_use] pub fn is_empty(&self) -> bool { - self.documents.is_empty() + lock_std(&self.documents).is_empty() } /// Open a document and track its state. @@ -201,15 +215,7 @@ impl DocumentTracker { /// Returns an error if: /// - Document limit is exceeded /// - File size limit is exceeded - pub fn open(&mut self, path: PathBuf, content: String) -> Result { - // Check document limit - if self.limits.max_documents > 0 && self.documents.len() >= self.limits.max_documents { - return Err(Error::DocumentLimitExceeded { - current: self.documents.len(), - max: self.limits.max_documents, - }); - } - + pub fn open(&self, path: PathBuf, content: String) -> Result { self.check_file_size(content.len() as u64)?; let uri = path_to_uri(&path); @@ -224,7 +230,19 @@ impl DocumentTracker { synced: HashMap::new(), }; - self.documents.insert(path, state); + // Check document limit and insert under a single lock acquisition so + // two concurrent `open` calls for different new paths can't both + // pass the check and jointly exceed the limit by one. Dropped + // explicitly right after the insert rather than at function return. + let mut documents = lock_std(&self.documents); + if self.limits.max_documents > 0 && documents.len() >= self.limits.max_documents { + return Err(Error::DocumentLimitExceeded { + current: documents.len(), + max: self.limits.max_documents, + }); + } + documents.insert(path, state); + drop(documents); Ok(uri) } @@ -233,8 +251,9 @@ impl DocumentTracker { /// Returns `None` if the document is not open. The updated content has no /// known disk provenance, so the next `ensure_open` call on this path /// will always re-verify by content compare rather than trusting a stat. - pub fn update(&mut self, path: &Path, content: String) -> Option { - if let Some(state) = self.documents.get_mut(path) { + pub fn update(&self, path: &Path, content: String) -> Option { + let mut documents = lock_std(&self.documents); + if let Some(state) = documents.get_mut(path) { state.version += 1; state.content = content; state.disk = None; @@ -257,11 +276,12 @@ impl DocumentTracker { /// Sets the disk snapshot for an already-tracked document. /// - /// A no-op if the path is no longer tracked; every call site holds the - /// tracker lock for the whole `ensure_open` call, so this should not - /// happen in practice, but it avoids an `unwrap`/`expect` on the lookup. - fn set_disk(&mut self, path: &Path, snap: DiskSync) { - if let Some(st) = self.documents.get_mut(path) { + /// A no-op if the path is no longer tracked; every call site runs under + /// the per-path lock for the whole `ensure_open` call, so this should + /// not happen in practice, but it avoids an `unwrap`/`expect` on the + /// lookup. + fn set_disk(&self, path: &Path, snap: DiskSync) { + if let Some(st) = lock_std(&self.documents).get_mut(path) { st.disk = Some(snap); } } @@ -269,18 +289,52 @@ impl DocumentTracker { /// Close a document and remove it from tracking. /// /// Returns the document state if it was open. - pub fn close(&mut self, path: &Path) -> Option { - self.documents.remove(path) + pub fn close(&self, path: &Path) -> Option { + lock_std(&self.documents).remove(path) } /// Close all documents. - pub fn close_all(&mut self) -> Vec { - self.documents.drain().map(|(_, state)| state).collect() + pub fn close_all(&self) -> Vec { + lock_std(&self.documents) + .drain() + .map(|(_, state)| state) + .collect() } - /// Iterate over the filesystem paths of all currently open documents. - pub fn open_paths(&self) -> impl Iterator { - self.documents.keys().map(PathBuf::as_path) + /// Snapshot of the filesystem paths of all currently open documents. + pub fn open_paths(&self) -> Vec { + lock_std(&self.documents).keys().cloned().collect() + } + + /// Acquire the per-path lock used by [`Self::ensure_open`], creating its + /// entry on first use. + /// + /// The map of per-path locks (`path_locks`) is itself locked only for + /// the map lookup/insert/remove — never across an `await` — so acquiring + /// one path's lock never blocks a concurrent acquisition for a different + /// path. Awaiting the returned path's own lock is what actually + /// serializes calls for the same path. + /// + /// The returned guard evicts its `path_locks` entry when dropped, but + /// only if no other caller is concurrently waiting on it (see + /// [`PathLockGuard`]'s `Drop` impl) — otherwise the map would grow by + /// one entry per distinct path ever opened, for the lifetime of the + /// process. + async fn lock_path(&self, path: &Path) -> PathLockGuard<'_> { + let arc = { + let mut locks = lock_std(&self.path_locks); + locks + .entry(path.to_path_buf()) + .or_insert_with(|| Arc::new(AsyncMutex::new(()))) + .clone() + }; + let guard = Arc::clone(&arc).lock_owned().await; + PathLockGuard { + path_locks: &self.path_locks, + path: path.to_path_buf(), + arc, + guard: Some(guard), + } } /// Ensure a document is open *for `server`*, opening it lazily if @@ -337,10 +391,13 @@ impl DocumentTracker { /// /// # Concurrency /// - /// This entire call must run under the document tracker's lock: it - /// assumes no other caller can observe or mutate state for the same path - /// concurrently, or duplicate `didOpen`/`didChange` notifications could - /// be sent for the same document. + /// Calls for the *same* `path` are serialized against each other (via + /// `lock_path`), so no two such calls can observe or mutate that + /// path's state concurrently -- this is what prevents duplicate + /// `didOpen`/`didChange` notifications for the same document. Calls for + /// *different* paths run fully concurrently: neither the per-path lock + /// nor the short, synchronous locks used to touch the shared document + /// map are ever held across this call's disk I/O or LSP notify. /// /// # Errors /// @@ -349,11 +406,12 @@ impl DocumentTracker { /// - The `didOpen`/`didChange` notification fails to send /// - Resource limits are exceeded pub async fn ensure_open( - &mut self, + &self, path: &Path, server: &ServerId, lsp_client: &LspClient, ) -> Result { + let _path_guard = self.lock_path(path).await; let decision = self.disk_phase(path).await?; self.sync_phase(path, server, lsp_client, decision).await } @@ -362,8 +420,8 @@ impl DocumentTracker { /// should be at, reading from disk only when necessary. Never sends any /// LSP notification and never returns early in a way that would skip the /// per-server sync phase -- see `ensure_open`'s docs. - async fn disk_phase(&mut self, path: &Path) -> Result { - if !self.documents.contains_key(path) { + async fn disk_phase(&self, path: &Path) -> Result { + if !lock_std(&self.documents).contains_key(path) { return self.disk_phase_new(path).await; } @@ -375,19 +433,25 @@ impl DocumentTracker { let mtime = meta.modified().ok(); let size = meta.len(); - let (uri, current_version, fast_path) = { - let Some(st) = self.documents.get(path) else { - return Err(Error::DocumentNotFound(path.to_path_buf())); - }; - let stat_matches = st.disk.is_some_and(|d| d.mtime == mtime && d.size == size); - let fast_path = match st.disk { - Some(d) if stat_matches && d.mtime_settled => true, - Some(d) if stat_matches && d.content_checked_at.elapsed() < DISK_CHECK_DEBOUNCE => { - true - } - _ => false, - }; - (st.uri.clone(), st.version, fast_path) + // `.map(...)` extracts an owned tuple from the lookup in a single + // statement, so the lock releases immediately rather than staying + // held while `fast_path` is computed. + let Some((uri, current_version, fast_path)) = + lock_std(&self.documents).get(path).map(|st| { + let stat_matches = st.disk.is_some_and(|d| d.mtime == mtime && d.size == size); + let fast_path = match st.disk { + Some(d) if stat_matches && d.mtime_settled => true, + Some(d) + if stat_matches && d.content_checked_at.elapsed() < DISK_CHECK_DEBOUNCE => + { + true + } + _ => false, + }; + (st.uri.clone(), st.version, fast_path) + }) + else { + return Err(Error::DocumentNotFound(path.to_path_buf())); }; if fast_path { return Ok(Decision::unchanged(uri, current_version)); @@ -401,11 +465,11 @@ impl DocumentTracker { content_checked_at: Instant::now(), }; - let unchanged = { - let Some(st) = self.documents.get(path) else { - return Err(Error::DocumentNotFound(path.to_path_buf())); - }; - fresh == st.content + let Some(unchanged) = lock_std(&self.documents) + .get(path) + .map(|st| fresh == st.content) + else { + return Err(Error::DocumentNotFound(path.to_path_buf())); }; if unchanged { @@ -424,7 +488,7 @@ impl DocumentTracker { /// Reads a not-yet-tracked file from disk and opens it in the tracker at /// version 1. No server has synced it yet, so the sync phase always /// sends `didOpen` regardless of which server calls next. - async fn disk_phase_new(&mut self, path: &Path) -> Result { + async fn disk_phase_new(&self, path: &Path) -> Result { let read_at = SystemTime::now(); let (content, mtime, size) = self.read_to_string_checked(path).await?; @@ -480,7 +544,7 @@ impl DocumentTracker { /// or nothing to `server` depending on its last-synced version, and /// commits the outcome only after the notification succeeds. async fn sync_phase( - &mut self, + &self, path: &Path, server: &ServerId, lsp_client: &LspClient, @@ -496,28 +560,27 @@ impl DocumentTracker { // Cheap check first: the common case (an already-synced document, // which is most tool calls against a file already open elsewhere) // must not pay for cloning the full document content only to - // discard it on the `up_to_date` return below. - let (up_to_date, is_first_open) = { - let Some(st) = self.documents.get(path) else { - return Err(Error::DocumentNotFound(path.to_path_buf())); - }; - let synced_version = st.synced.get(server).copied(); - ( - synced_version.is_some_and(|v| v >= target_version), - synced_version.is_none(), - ) + // discard it on the `up_to_date` return below. `.map(...)` extracts + // an owned value from the lookup so the lock is released at the end + // of this statement rather than held across the checks that follow. + let Some(synced_version) = lock_std(&self.documents) + .get(path) + .map(|st| st.synced.get(server).copied()) + else { + return Err(Error::DocumentNotFound(path.to_path_buf())); }; + let up_to_date = synced_version.is_some_and(|v| v >= target_version); + let is_first_open = synced_version.is_none(); if up_to_date { return Ok(uri); } - let (language_id, text) = { - let Some(st) = self.documents.get(path) else { - return Err(Error::DocumentNotFound(path.to_path_buf())); - }; + let Some((language_id, text)) = lock_std(&self.documents).get(path).map(|st| { let text = fresh_content.clone().unwrap_or_else(|| st.content.clone()); (st.language_id.clone(), text) + }) else { + return Err(Error::DocumentNotFound(path.to_path_buf())); }; let notify_result = if is_first_open { @@ -561,17 +624,24 @@ impl DocumentTracker { // another server already synced successfully, the path stays // tracked for that server's sake; this server's `synced` entry // simply stays absent/stale, so its own next call retries. - let first_ever_sync = self - .documents + // Two short lock scopes rather than one held across the + // conditional `remove`: safe because `ensure_open`'s per-path + // lock already serializes every caller for this path, so + // nothing else can observe or mutate its `synced` map between + // them. + let first_ever_sync = lock_std(&self.documents) .get(path) .is_some_and(|st| st.synced.is_empty()); if is_first_open && first_ever_sync { - self.documents.remove(path); + lock_std(&self.documents).remove(path); } return Err(err); } - let Some(st) = self.documents.get_mut(path) else { + // Dropped explicitly right after the commit, rather than staying + // alive (unused) until the function returns. + let mut documents = lock_std(&self.documents); + let Some(st) = documents.get_mut(path) else { return Err(Error::DocumentNotFound(path.to_path_buf())); }; if let Some(fresh) = fresh_content { @@ -580,11 +650,51 @@ impl DocumentTracker { st.disk = snap; } st.synced.insert(server.clone(), target_version); + drop(documents); Ok(uri) } } +/// RAII guard for the per-path lock acquired by +/// [`DocumentTracker::lock_path`]. +/// +/// Holds an `OwnedMutexGuard` on the path's `Arc>>` for as +/// long as the guard is alive, serializing `ensure_open` calls for that +/// path. On drop, evicts the `path_locks` map entry if (and only if) no +/// other caller holds a clone of the same `Arc` -- see the `Drop` impl for +/// why that check is race-free. +struct PathLockGuard<'a> { + path_locks: &'a StdMutex>>>, + path: PathBuf, + arc: Arc>, + guard: Option>, +} + +impl Drop for PathLockGuard<'_> { + fn drop(&mut self) { + // Unlock first so a task waiting on `arc.lock_owned()` can proceed + // as soon as possible, rather than also waiting on `path_locks`. + self.guard.take(); + + let mut locks = lock_std(self.path_locks); + // Checked only after `self.guard` -- and the extra internal `Arc` + // clone it held -- was already dropped above, so what's left here is: + // this task's own `self.arc`, the map's entry, and one more + // reference for every *other* task that has already looked up this + // same entry in `lock_path` (each holds its own clone continuously + // from before that lookup until its own `Drop` runs this same check) + // but hasn't finished dropping yet. A `strong_count` of 2 means no + // such task exists, so it's safe to evict; any later caller just + // creates a fresh entry. Leaving it forever would instead grow this + // map by one entry per distinct path ever opened, for the process's + // lifetime. + if Arc::strong_count(&self.arc) <= 2 { + locks.remove(&self.path); + } + } +} + /// Outcome of `DocumentTracker::disk_phase`: the version `ensure_open`'s /// caller should end up synced to, and -- only when this call detected an /// as-yet-uncommitted content change -- the content and disk snapshot to @@ -713,7 +823,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path = PathBuf::from("/test/file.rs"); assert!(!tracker.is_open(&path)); @@ -745,7 +855,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(limits, map); + let tracker = DocumentTracker::new(limits, map); // First two documents should succeed tracker @@ -769,7 +879,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(limits, map); + let tracker = DocumentTracker::new(limits, map); // Small file should succeed tracker @@ -808,7 +918,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(limits, map); + let tracker = DocumentTracker::new(limits, map); // Should allow many documents when limit is 0 for i in 0..200 { @@ -850,7 +960,7 @@ mod tests { #[test] fn test_update_nonexistent_document() { let map = HashMap::new(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path = PathBuf::from("/test/nonexistent.rs"); let version = tracker.update(&path, "new content".to_string()); @@ -863,7 +973,7 @@ mod tests { #[test] fn test_close_nonexistent_document() { let map = HashMap::new(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path = PathBuf::from("/test/nonexistent.rs"); let state = tracker.close(&path); @@ -878,7 +988,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); tracker .open(PathBuf::from("/test/file1.rs"), "content1".to_string()) @@ -915,7 +1025,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path = PathBuf::from("/test/versioned.rs"); tracker.open(path.clone(), "v1".to_string()).unwrap(); @@ -1116,7 +1226,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path1 = PathBuf::from("/test/file1.rs"); let path2 = PathBuf::from("/test/file2.rs"); @@ -1142,7 +1252,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path = PathBuf::from("/test/empty.rs"); tracker.open(path.clone(), String::new()).unwrap(); @@ -1155,7 +1265,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path = PathBuf::from("/test/unicode.rs"); let content = "fn テスト() { println!(\"こんにちは\"); }"; @@ -1172,7 +1282,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(limits, map); + let tracker = DocumentTracker::new(limits, map); for i in 0..5 { tracker @@ -1198,7 +1308,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(limits, map); + let tracker = DocumentTracker::new(limits, map); let exact_size_content = "x".repeat(100); tracker @@ -1260,7 +1370,7 @@ mod tests { let mut map = HashMap::new(); map.insert("nu".to_string(), "nushell".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path = PathBuf::from("/test/script.nu"); tracker @@ -1276,7 +1386,7 @@ mod tests { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); let path = PathBuf::from("/test/main.rs"); tracker .open(path.clone(), "fn main() {}".to_string()) @@ -1351,29 +1461,29 @@ mod tests { #[test] fn test_open_paths_empty_tracker() { let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); - assert_eq!(tracker.open_paths().count(), 0); + assert_eq!(tracker.open_paths().len(), 0); } #[test] fn test_open_paths_populated_tracker() { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); tracker.open(PathBuf::from("/a.rs"), String::new()).unwrap(); tracker.open(PathBuf::from("/b.rs"), String::new()).unwrap(); - let mut paths: Vec<_> = tracker.open_paths().collect(); + let mut paths = tracker.open_paths(); paths.sort(); - assert_eq!(paths, [Path::new("/a.rs"), Path::new("/b.rs")]); + assert_eq!(paths, [PathBuf::from("/a.rs"), PathBuf::from("/b.rs")]); } #[test] fn test_open_paths_after_close() { let mut map = HashMap::new(); map.insert("rs".to_string(), "rust".to_string()); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), map); + let tracker = DocumentTracker::new(ResourceLimits::default(), map); tracker.open(PathBuf::from("/a.rs"), String::new()).unwrap(); tracker.close(Path::new("/a.rs")); - assert_eq!(tracker.open_paths().count(), 0); + assert_eq!(tracker.open_paths().len(), 0); } // ------------------------------------------------------------------ @@ -1510,7 +1620,7 @@ mod tests { set_mtime(&path, settled_past()); let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); let uri1 = tracker .ensure_open(&path, &ServerId::from("rust"), &client) @@ -1535,7 +1645,7 @@ mod tests { set_mtime(&path, settled_past()); let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1561,7 +1671,7 @@ mod tests { // Leave the mtime at "now" (racy) rather than backdating it. let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1595,7 +1705,7 @@ mod tests { set_mtime(&path, settled_past()); let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1627,7 +1737,7 @@ mod tests { set_mtime(&path, settled_past()); let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1654,7 +1764,7 @@ mod tests { // Racy: leave the mtime at "now". let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1689,7 +1799,7 @@ mod tests { set_mtime(&path, settled_past()); let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1718,7 +1828,7 @@ mod tests { max_file_size: 10, }; let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(limits, HashMap::new()); + let tracker = DocumentTracker::new(limits, HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1746,7 +1856,7 @@ mod tests { max_file_size: 0, }; let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(limits, HashMap::new()); + let tracker = DocumentTracker::new(limits, HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1773,7 +1883,7 @@ mod tests { set_mtime(&path, settled_past()); let (client, _server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1802,7 +1912,7 @@ mod tests { let notify_will_fail = client.clone(); client.shutdown().await.unwrap(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); let result = tracker .ensure_open(&path, &ServerId::from("rust"), ¬ify_will_fail) .await; @@ -1823,7 +1933,7 @@ mod tests { set_mtime(&path, settled_past()); let (client, mut server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("rust"), &client) .await @@ -1869,7 +1979,7 @@ mod tests { let (client_a, mut server_a) = fake_lsp_client(); let (client_b, mut server_b) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); let id_a = ServerId::from("server-a"); let id_b = ServerId::from("server-b"); @@ -1902,7 +2012,7 @@ mod tests { let (client_a, _server_a) = fake_lsp_client(); let (client_b, mut server_b) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); tracker .ensure_open(&path, &ServerId::from("server-a"), &client_a) @@ -1936,7 +2046,7 @@ mod tests { set_mtime(&path, settled_past()); let (client, mut server) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); let id = ServerId::from("rust"); tracker.ensure_open(&path, &id, &client).await.unwrap(); @@ -1964,7 +2074,7 @@ mod tests { let (client_a, _server_a) = fake_lsp_client(); let (client_b, _server_b) = fake_lsp_client(); - let mut tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); + let tracker = DocumentTracker::new(ResourceLimits::default(), HashMap::new()); let id_a = ServerId::from("server-a"); let id_b = ServerId::from("server-b"); @@ -2004,4 +2114,178 @@ mod tests { assert_eq!(tracker.get(&path).unwrap().synced.get(&id_a), Some(&2)); assert_eq!(tracker.get(&path).unwrap().synced.get(&id_b), Some(&1)); } + + // ------------------------------------------------------------------ + // ensure_open concurrency (issue #227) + // ------------------------------------------------------------------ + + /// Regression for #227: `ensure_open` for one path must not block + /// `ensure_open` for an unrelated path, even while the first call is + /// stuck inside its own disk I/O. + /// + /// Simulated with a FIFO rather than a timing assumption: opening it for + /// read blocks deterministically until a writer connects, so path A's + /// `ensure_open` is guaranteed to still be in progress when path B's + /// runs. Under the old design (a single lock spanning all of + /// `ensure_open`, including disk I/O), path B would hang until path A's + /// FIFO is unblocked below; the per-path lock added here must let it + /// through immediately instead. + #[cfg(unix)] + #[tokio::test] + async fn test_ensure_open_different_paths_do_not_serialize() { + let dir = TempDir::new().unwrap(); + let path_a = dir.path().join("a.rs"); + let path_b = dir.path().join("b.rs"); + + std::fs::write(&path_b, "fn b() {}").unwrap(); + set_mtime(&path_b, settled_past()); + + let status = std::process::Command::new("mkfifo") + .arg(&path_a) + .status() + .unwrap(); + assert!(status.success(), "mkfifo must succeed to set up this test"); + + let (client_a, _server_a) = fake_lsp_client(); + let (client_b, _server_b) = fake_lsp_client(); + let tracker = Arc::new(DocumentTracker::new( + ResourceLimits::default(), + HashMap::new(), + )); + + // Spawned so it can genuinely block on the FIFO's open() while the + // rest of this test proceeds concurrently on the same runtime. + let tracker_for_a = Arc::clone(&tracker); + let path_a_for_task = path_a.clone(); + let handle_a = tokio::spawn(async move { + tracker_for_a + .ensure_open(&path_a_for_task, &ServerId::from("server-a"), &client_a) + .await + }); + + // Give the spawned task a chance to actually reach the FIFO's + // blocking open() before racing it against path B below. + tokio::time::sleep(Duration::from_millis(200)).await; + + // A `timeout` error here means path B is blocked by path A's stuck + // ensure_open -- the exact regression #227 fixes. + tokio::time::timeout( + Duration::from_secs(5), + tracker.ensure_open(&path_b, &ServerId::from("server-b"), &client_b), + ) + .await + .unwrap() + .unwrap(); + + // Unblock A: opening the FIFO for writing lets its open() proceed, + // and closing the write end (at the end of this call) delivers EOF + // to the read it's waiting to finish. + let path_a_writer = path_a.clone(); + tokio::task::spawn_blocking(move || { + std::fs::write(path_a_writer, "fn a() {}").unwrap(); + }) + .await + .unwrap(); + + handle_a.await.unwrap().unwrap(); + assert_eq!(tracker.get(&path_a).unwrap().content, "fn a() {}"); + } + + /// Regression for #227: N concurrent `ensure_open` calls for the same + /// path and the same server must still collapse into exactly one + /// `didOpen` -- the per-path lock introduced to let different paths run + /// concurrently must not weaken the existing same-path serialization + /// that prevents duplicate opens. + #[tokio::test] + async fn test_ensure_open_concurrent_same_path_single_didopen() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("a.rs"); + std::fs::write(&path, "fn main() {}").unwrap(); + set_mtime(&path, settled_past()); + + let (client, mut server) = fake_lsp_client(); + let tracker = Arc::new(DocumentTracker::new( + ResourceLimits::default(), + HashMap::new(), + )); + let id = ServerId::from("rust"); + + let mut handles = Vec::new(); + for _ in 0..8 { + let tracker = Arc::clone(&tracker); + let client = client.clone(); + let path = path.clone(); + let id = id.clone(); + handles.push(tokio::spawn(async move { + tracker.ensure_open(&path, &id, &client).await + })); + } + for handle in handles { + handle.await.unwrap().unwrap(); + } + + let mut wire = BufReader::new(&mut server.write_stdout); + let opened = read_framed_message(&mut wire).await; + assert_eq!(opened["method"], "textDocument/didOpen"); + + // No further notification should have been queued -- proves the 8 + // concurrent callers collapsed into exactly one `didOpen`. + let extra = + tokio::time::timeout(Duration::from_millis(200), read_framed_message(&mut wire)).await; + assert!( + extra.is_err(), + "expected no additional notification after the single didOpen" + ); + + assert_eq!(tracker.get(&path).unwrap().synced.get(&id), Some(&1)); + assert_eq!(tracker.get(&path).unwrap().version, 1); + } + + /// Regression for #227: `lock_path`'s guard must evict its `path_locks` + /// entry once no caller is left waiting on it, or the map grows by one + /// entry per distinct path ever opened for the lifetime of the process. + /// Exercises three concurrent distinct paths (not just the two used in + /// `test_ensure_open_different_paths_do_not_serialize`) to rule out an + /// eviction bug that only manifests with more than two live entries. + #[tokio::test] + async fn test_ensure_open_path_locks_evicted_after_completion() { + let dir = TempDir::new().unwrap(); + let paths: Vec<_> = ["a.rs", "b.rs", "c.rs"] + .iter() + .map(|name| dir.path().join(name)) + .collect(); + for path in &paths { + std::fs::write(path, "fn f() {}").unwrap(); + set_mtime(path, settled_past()); + } + + let tracker = Arc::new(DocumentTracker::new( + ResourceLimits::default(), + HashMap::new(), + )); + let id = ServerId::from("rust"); + + let mut handles = Vec::new(); + let mut servers = Vec::new(); + for path in paths.clone() { + let tracker = Arc::clone(&tracker); + let (client, server) = fake_lsp_client(); + servers.push(server); + let id = id.clone(); + handles.push(tokio::spawn(async move { + tracker.ensure_open(&path, &id, &client).await + })); + } + for handle in handles { + handle.await.unwrap().unwrap(); + } + drop(servers); + + assert!( + lock_std(&tracker.path_locks).is_empty(), + "path_locks must be fully evicted once every ensure_open call \ + for every path has completed, otherwise the map grows \ + unbounded for the lifetime of the process" + ); + } } diff --git a/crates/mcpls-core/src/bridge/translator.rs b/crates/mcpls-core/src/bridge/translator.rs index c0c11b0a..8528bca5 100644 --- a/crates/mcpls-core/src/bridge/translator.rs +++ b/crates/mcpls-core/src/bridge/translator.rs @@ -2,7 +2,7 @@ use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; -use std::sync::{Arc, Mutex as StdMutex, MutexGuard, PoisonError}; +use std::sync::{Arc, Mutex as StdMutex}; use lsp_types::{ CallHierarchyIncomingCall, CallHierarchyIncomingCallsParams, CallHierarchyItem, @@ -17,28 +17,15 @@ use lsp_types::{ WorkspaceSymbolParams as LspWorkspaceSymbolParams, }; use serde::{Deserialize, Serialize}; -use tokio::sync::Mutex as AsyncMutex; use tokio::time::Duration; use super::state::{ResourceLimits, detect_language, path_to_uri}; -use super::{DiagnosticInfo, DocumentTracker, NotificationCache}; +use super::{DiagnosticInfo, DocumentTracker, NotificationCache, lock_std}; use crate::bridge::encoding::mcp_to_lsp_position; use crate::config::{ServerId, ToolKind, ToolRouter, base_language_id}; use crate::error::{Error, Result}; use crate::lsp::{LspClient, LspServer}; -/// Lock a `std::sync::Mutex`, recovering the guard if a previous holder -/// panicked while holding it. -/// -/// Every lock guarded this way protects a short, synchronous, panic-free -/// critical section (a `HashMap`/`HashSet` lookup or insert), so poisoning -/// can only happen if an unrelated bug already panicked; refusing to unwind -/// the whole process a second time over stale poisoning is preferable to -/// deadlocking future calls. -fn lock_std(mutex: &StdMutex) -> MutexGuard<'_, T> { - mutex.lock().unwrap_or_else(PoisonError::into_inner) -} - /// Translator handles MCP tool calls by converting them to LSP requests. /// /// All fields use interior mutability so `Translator` can be shared via a @@ -49,11 +36,11 @@ fn lock_std(mutex: &StdMutex) -> MutexGuard<'_, T> { /// section that touches it. In particular, the actual LSP request/response /// round trip (`client.request(...)`) always runs with no lock held. /// -/// The `document_tracker` lock is the one exception worth calling out: it is -/// still a single lock shared across all languages and paths, and -/// `prepare_document`'s call into `ensure_open` holds it across that -/// document's disk I/O and `textDocument/didOpen` notify — see the -/// `TODO(critic-S2)` there for why that is bounded and out of scope here. +/// `document_tracker` is no exception: `DocumentTracker` locks its own state +/// per-path internally (see its docs), so `prepare_document`'s call into +/// `ensure_open` never holds a lock shared across unrelated paths or +/// languages while it does that document's disk I/O and +/// `textDocument/didOpen`/`didChange` notify. #[derive(Debug)] pub struct Translator { /// LSP clients indexed by routing identity. Locked only for the map @@ -61,8 +48,8 @@ pub struct Translator { lsp_clients: Arc>>, /// LSP servers indexed by routing identity (held for lifetime management). lsp_servers: Arc>>, - /// Document state tracker. Locked only for `ensure_open`. - document_tracker: Arc>, + /// Document state tracker. Locks its own state internally, per path. + document_tracker: Arc, /// Allowed workspace roots for path validation. Read-only after `serve()` /// setup, so no lock is needed. workspace_roots: Arc>, @@ -90,10 +77,10 @@ impl Translator { Self { lsp_clients: Arc::new(StdMutex::new(HashMap::new())), lsp_servers: Arc::new(StdMutex::new(HashMap::new())), - document_tracker: Arc::new(AsyncMutex::new(DocumentTracker::new( + document_tracker: Arc::new(DocumentTracker::new( ResourceLimits::default(), HashMap::new(), - ))), + )), workspace_roots: Arc::new(Vec::new()), extension_map: Arc::new(HashMap::new()), expected_servers: Arc::new(StdMutex::new(HashSet::new())), @@ -157,10 +144,10 @@ impl Translator { /// shared, so this replaces the `Arc`-wrapped fields wholesale. #[must_use] pub fn with_extensions(mut self, extension_map: HashMap) -> Self { - self.document_tracker = Arc::new(AsyncMutex::new(DocumentTracker::new( + self.document_tracker = Arc::new(DocumentTracker::new( ResourceLimits::default(), extension_map.clone(), - ))); + )); self.extension_map = Arc::new(extension_map); self } @@ -182,19 +169,15 @@ impl Translator { } /// Snapshot of currently open document paths, used for MCP resource listing. - pub async fn open_document_paths(&self) -> Vec { - self.document_tracker - .lock() - .await - .open_paths() - .map(Path::to_path_buf) - .collect() + #[must_use] + pub fn open_document_paths(&self) -> Vec { + self.document_tracker.open_paths() } /// Whether a document is currently tracked as open. #[must_use] - pub async fn is_document_open(&self, path: &Path) -> bool { - self.document_tracker.lock().await.is_open(path) + pub fn is_document_open(&self, path: &Path) -> bool { + self.document_tracker.is_open(path) } // TODO: These methods will be implemented in Phase 3-5 @@ -727,23 +710,19 @@ impl Translator { /// /// This is the "prepare" phase shared by every LSP-round-trip handler: /// it validates the path, selects the client via [`Self::get_client_for_file`], - /// and locks the document tracker only for `ensure_open`. The returned - /// client and URI are owned values, so the caller can issue the actual - /// LSP request (the "execute" phase) without holding any lock across the - /// network round trip — that is the lock this function's callers care - /// about, and it is always released before this function returns. + /// and calls `ensure_open`, which locks the document tracker's state only + /// for the given path. The returned client and URI are owned values, so + /// the caller can issue the actual LSP request (the "execute" phase) + /// without holding any lock across the network round trip. /// - /// The `document_tracker` lock itself, however, is held across /// `ensure_open`'s own awaits (a `stat`, optionally a re-read of the - /// file, and the `textDocument/didOpen`/`didChange` notify), and that - /// lock is shared by every language and every path. In the common case - /// this is bounded by the 250ms disk-check debounce, but it is not - /// per-path. - // TODO(critic-S2): scope this lock per path (e.g. a keyed mutex) so a - // wedged language server's `notify(...)` — a bounded-channel send with - // no timeout — can't stall `ensure_open` for unrelated files/languages. - // Preserve the anti-duplicate-`didOpen` invariant documented on - // `ensure_open` if this changes. + /// file, and the `textDocument/didOpen`/`didChange` notify) run under a + /// lock scoped to `validated_path` alone — see [`DocumentTracker::ensure_open`] + /// — so a slow or wedged language server cannot stall `prepare_document` + /// calls for unrelated files. (Per-tool routing, #228, means the same + /// file can be routed to more than one server; a wedged server-A notify + /// still holds this path's lock and can therefore delay a healthy + /// server-B call for that *same* file.) async fn prepare_document( &self, file_path: &str, @@ -754,8 +733,6 @@ impl Translator { let (server_id, client) = self.get_client_for_file(&validated_path, tool)?; let uri = self .document_tracker - .lock() - .await .ensure_open(&validated_path, &server_id, &client) .await?; Ok((client, uri)) @@ -3773,9 +3750,9 @@ mod tests { #[tokio::test] async fn test_concurrent_ensure_open_same_path_sends_single_did_open() { - // Regression test: `ensure_open` must run entirely under the - // document_tracker lock so concurrent handler calls for the SAME path - // can't both observe "not open yet" and both send didOpen. + // Regression test: concurrent handler calls for the SAME path must + // serialize on that path's `ensure_open` lock (see `DocumentTracker::lock_path`) + // so they can't both observe "not open yet" and both send didOpen. let dir = TempDir::new().unwrap(); let mut extensions = HashMap::new(); extensions.insert("aa".to_string(), "lang_a".to_string()); diff --git a/crates/mcpls-core/src/config/mod.rs b/crates/mcpls-core/src/config/mod.rs index a19d6d81..eaadbece 100644 --- a/crates/mcpls-core/src/config/mod.rs +++ b/crates/mcpls-core/src/config/mod.rs @@ -293,6 +293,29 @@ fn default_language_extensions() -> Vec { ] } +/// Trust level applied to a `./mcpls.toml` discovered relative to the +/// process's current working directory. +/// +/// A CWD-discovered project-local config is not the same trust tier as an +/// explicit `--config`/`MCPLS_CONFIG` path: it can be planted by whoever +/// controls the checked-out repository, and it controls the `command` and +/// `args` mcpls spawns as well as `[workspace]` (which can redirect the +/// spawn target via `roots` or drive a filesystem-walk `DoS` via +/// `heuristics_max_depth`). [`ServerConfig::load`] treats it as +/// [`Untrusted`](Self::Untrusted) by default; callers that want it honored +/// must opt in via [`ServerConfig::load_with_trust`]. +/// +/// An explicitly passed `--config`/`MCPLS_CONFIG` path is unaffected by this +/// enum and is always trusted: naming a path is itself the user's consent. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ProjectConfigTrust { + /// Ignore a CWD-discovered `./mcpls.toml` entirely; fall through to the + /// global config tier or built-in defaults. + Untrusted, + /// Load a CWD-discovered `./mcpls.toml` normally. + Trusted, +} + impl ServerConfig { /// Build the effective extension map used for language detection. /// @@ -314,29 +337,79 @@ impl ServerConfig { map } - /// Load configuration from the default path. + /// Load configuration from the default path, treating a CWD-discovered + /// `./mcpls.toml` as untrusted. /// /// Default paths checked in order: - /// 1. `$MCPLS_CONFIG` environment variable - /// 2. `./mcpls.toml` (current directory) + /// 1. `$MCPLS_CONFIG` environment variable (always trusted) + /// 2. `./mcpls.toml` (current directory) — **skipped**; see + /// [`load_with_trust`](Self::load_with_trust) to opt in /// 3. `~/.config/mcpls/mcpls.toml` (Linux/macOS) /// 4. `%APPDATA%\mcpls\mcpls.toml` (Windows) /// /// If no configuration file exists, creates a default configuration file /// in the user's config directory with all default language extensions. /// + /// This is a thin wrapper around + /// [`load_with_trust(ProjectConfigTrust::Untrusted)`](Self::load_with_trust) — + /// the safe default for library callers that haven't made a trust + /// decision. + /// /// # Errors /// /// Returns an error if parsing an existing config fails. /// If config creation fails, returns default config with graceful degradation. pub fn load() -> Result { + Self::load_with_trust(ProjectConfigTrust::Untrusted) + } + + /// Load configuration from the default path, with explicit control over + /// whether a CWD-discovered `./mcpls.toml` is honored. + /// + /// Behaves like [`load`](Self::load), except a `./mcpls.toml` found in + /// the current directory is only loaded when `trust` is + /// [`ProjectConfigTrust::Trusted`]. When untrusted, the file is skipped + /// entirely (including its `[workspace]` section) and a warning is + /// logged naming the ignored path; discovery falls through to the + /// global config tier or built-in defaults, so project-marker + /// heuristics (e.g. `Cargo.toml` → rust-analyzer) still apply normally. + /// + /// `$MCPLS_CONFIG` and an explicit path are unaffected by `trust` and + /// are always loaded: naming a path is itself the user's consent. + /// + /// # Errors + /// + /// Returns an error if parsing an existing config fails. + /// If config creation fails, returns default config with graceful degradation. + pub fn load_with_trust(trust: ProjectConfigTrust) -> Result { + // This `$MCPLS_CONFIG` check is unreachable from the `mcpls` binary: + // `crates/mcpls-cli/src/args.rs` already binds `env = "MCPLS_CONFIG"` + // to `--config`, so the CLI resolves that variable before `load`/ + // `load_with_trust` is ever called. It only fires for library + // callers that invoke this function directly without going through + // `Args`. The actual, CLI-enforced guarantee that `$MCPLS_CONFIG` is + // always trusted lives in `main.rs`'s `--config` branch, not here. if let Ok(path) = std::env::var("MCPLS_CONFIG") { return Self::load_from(Path::new(&path)); } let local_config = PathBuf::from("mcpls.toml"); if local_config.exists() { - return Self::load_from(&local_config); + match trust { + ProjectConfigTrust::Trusted => return Self::load_from(&local_config), + ProjectConfigTrust::Untrusted => { + let display_path = local_config.canonicalize().unwrap_or_else(|_| { + std::env::current_dir() + .map_or_else(|_| local_config.clone(), |cwd| cwd.join(&local_config)) + }); + tracing::warn!( + "ignoring untrusted project-local config at {}; pass \ + --trust-project-config (or set MCPLS_TRUST_PROJECT_CONFIG=true) to \ + load it", + display_path.display() + ); + } + } } if let Some(config_dir) = dirs::config_dir() { @@ -1053,9 +1126,66 @@ mod tests { assert_eq!(config.lsp_servers[0].language_id, "rust"); } + // These tests mutate the process-wide CWD via `set_current_dir`, so they + // must not run concurrently with each other or with any other test that + // relies on CWD (e.g. via a bare `load()`/`load_with_trust()` call). + // Nextest runs each test in its own process, but `cargo test` in-process + // would race; guard with a mutex. + static CWD_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + #[test] + fn test_load_ignores_untrusted_project_local_config() { + // `ServerConfig::default()` (what untrusted discovery falls back to + // once neither an untrusted local file nor a global config apply) + // still exposes rust-analyzer via built-in project-marker + // heuristics — see `test_default_config` above, which already + // covers this without any filesystem interaction. This test only + // needs to prove the planted attacker file's content never leaks + // through `load()`. + let _guard = CWD_LOCK.lock().unwrap(); + let original_dir = std::env::current_dir().unwrap(); + + let tmp_dir = TempDir::new().unwrap(); + let config_path = tmp_dir.path().join("mcpls.toml"); + + // A marker language id / root that cannot collide with either the + // built-in defaults or a machine-local global config, so this + // assertion holds regardless of what `load()` actually falls + // through to (built-in defaults on a clean machine, or the + // machine's own customized global config in CI/dev environments). + let custom_toml = r#" + [workspace] + roots = ["/should-never-load-attacker-path"] + + [[lsp_servers]] + language_id = "definitely-not-a-real-language-marker" + command = "rm" + args = ["-rf", "/"] + "#; + + fs::write(&config_path, custom_toml).unwrap(); + + std::env::set_current_dir(tmp_dir.path()).unwrap(); + let config = ServerConfig::load().unwrap(); + std::env::set_current_dir(&original_dir).unwrap(); + + assert!( + !config + .workspace + .roots + .contains(&PathBuf::from("/should-never-load-attacker-path")) + ); + assert!( + !config + .lsp_servers + .iter() + .any(|s| s.language_id == "definitely-not-a-real-language-marker") + ); + } + #[test] - fn test_load_does_not_overwrite_existing_config() { - // Save original directory to restore it after the test + fn test_load_with_trust_loads_trusted_project_local_config() { + let _guard = CWD_LOCK.lock().unwrap(); let original_dir = std::env::current_dir().unwrap(); let tmp_dir = TempDir::new().unwrap(); @@ -1073,14 +1203,47 @@ mod tests { fs::write(&config_path, custom_toml).unwrap(); std::env::set_current_dir(tmp_dir.path()).unwrap(); - let config = ServerConfig::load().unwrap(); + let config = ServerConfig::load_with_trust(ProjectConfigTrust::Trusted).unwrap(); + std::env::set_current_dir(&original_dir).unwrap(); assert_eq!(config.workspace.roots, vec![PathBuf::from("/custom/path")]); assert_eq!(config.lsp_servers.len(), 1); assert_eq!(config.lsp_servers[0].language_id, "python"); + } - // Restore original directory to avoid affecting other tests - std::env::set_current_dir(original_dir).unwrap(); + #[test] + fn test_load_with_trust_untrusted_ignores_workspace_and_servers() { + let _guard = CWD_LOCK.lock().unwrap(); + let original_dir = std::env::current_dir().unwrap(); + + let tmp_dir = TempDir::new().unwrap(); + let config_path = tmp_dir.path().join("mcpls.toml"); + + let custom_toml = r#" + [workspace] + roots = ["/attacker/controlled"] + heuristics_max_depth = 999999 + + [[lsp_servers]] + language_id = "evil" + command = "rm" + args = ["-rf", "/"] + "#; + + fs::write(&config_path, custom_toml).unwrap(); + + std::env::set_current_dir(tmp_dir.path()).unwrap(); + let config = ServerConfig::load_with_trust(ProjectConfigTrust::Untrusted).unwrap(); + std::env::set_current_dir(&original_dir).unwrap(); + + assert!( + !config + .workspace + .roots + .contains(&PathBuf::from("/attacker/controlled")) + ); + assert_ne!(config.workspace.heuristics_max_depth, 999_999); + assert!(!config.lsp_servers.iter().any(|s| s.language_id == "evil")); } #[test] diff --git a/crates/mcpls-core/src/lib.rs b/crates/mcpls-core/src/lib.rs index 90e2be0e..7aabed67 100644 --- a/crates/mcpls-core/src/lib.rs +++ b/crates/mcpls-core/src/lib.rs @@ -46,7 +46,7 @@ use std::sync::Arc; use bridge::resources::make_uri; use bridge::{NotificationCache, ResourceSubscriptions, Translator}; -pub use config::ServerConfig; +pub use config::{ProjectConfigTrust, ServerConfig}; use config::{ServerId, ToolRouter}; pub use error::Error; use lsp::{LspNotification, LspServer, ServerInitConfig}; diff --git a/crates/mcpls-core/src/mcp/server.rs b/crates/mcpls-core/src/mcp/server.rs index c822e5ae..55f88b69 100644 --- a/crates/mcpls-core/src/mcp/server.rs +++ b/crates/mcpls-core/src/mcp/server.rs @@ -566,7 +566,7 @@ impl ServerHandler for McplsServer { ) -> Result { // TODO(critic-S5): paginate when max_documents == 0 (unlimited mode can produce // very large single-page responses that may exceed transport buffers). - let open_paths = self.context.translator.open_document_paths().await; + let open_paths = self.context.translator.open_document_paths(); let resources: Vec<_> = open_paths .iter() .filter_map(|path| { @@ -1265,12 +1265,7 @@ mod tests { #[tokio::test] async fn test_list_resources_returns_empty_when_no_open_documents() { let server = create_test_server(); - let empty = server - .context - .translator - .open_document_paths() - .await - .is_empty(); + let empty = server.context.translator.open_document_paths().is_empty(); assert!(empty); } diff --git a/crates/mcpls-core/tests/integration/basic_tests.rs b/crates/mcpls-core/tests/integration/basic_tests.rs index 0c96b100..277b31c3 100644 --- a/crates/mcpls-core/tests/integration/basic_tests.rs +++ b/crates/mcpls-core/tests/integration/basic_tests.rs @@ -8,10 +8,10 @@ use crate::common::test_utils::{ config_fixture_path, rust_analyzer_available, rust_workspace_path, }; -#[tokio::test] -async fn test_translator_creation() { +#[test] +fn test_translator_creation() { let translator = Translator::new(); - assert!(translator.open_document_paths().await.is_empty()); + assert!(translator.open_document_paths().is_empty()); } #[test] @@ -65,13 +65,13 @@ fn test_workspace_roots_configuration() { translator.set_workspace_roots(roots); } -#[tokio::test] -async fn test_document_tracker_lazy_opening() { +#[test] +fn test_document_tracker_lazy_opening() { let translator = Translator::new(); let test_file = rust_workspace_path().join("src/lib.rs"); assert!( - !translator.is_document_open(&test_file).await, + !translator.is_document_open(&test_file), "Document should not be open initially" ); } diff --git a/docs/user-guide/configuration.md b/docs/user-guide/configuration.md index 640e4fd7..fbd4ae80 100644 --- a/docs/user-guide/configuration.md +++ b/docs/user-guide/configuration.md @@ -8,9 +8,47 @@ mcpls uses TOML format for configuration. The file can be placed in several loca 1. Path specified by `--config` flag 2. `$MCPLS_CONFIG` environment variable -3. `./mcpls.toml` (current directory) +3. `./mcpls.toml` (current directory) — **only loaded with `--trust-project-config`** (or + `MCPLS_TRUST_PROJECT_CONFIG=true`); see [Trusting a Project-Local Config](#trusting-a-project-local-config) 4. `~/.config/mcpls/mcpls.toml` (user config directory) +### Trusting a Project-Local Config + +A `mcpls.toml` discovered in the current directory controls which command mcpls +spawns as an LSP server (and other workspace settings), so mcpls does not load it +automatically. Running `mcpls` inside an untrusted checkout must not execute +commands from that checkout without explicit consent. + +To load a project-local `mcpls.toml`, opt in explicitly: + +```bash +mcpls --trust-project-config +# or +MCPLS_TRUST_PROJECT_CONFIG=true mcpls +``` + +Without this flag, a `./mcpls.toml` in the current directory is ignored (a warning +is logged naming the ignored path) and mcpls falls through to the user config +directory or built-in defaults — including built-in project-marker heuristics, so +e.g. a `Cargo.toml` in the workspace still spawns rust-analyzer. An explicit +`--config ` or `$MCPLS_CONFIG` is always trusted, since naming a path is +itself the user's consent. + +> [!WARNING] +> `--trust-project-config` (and `MCPLS_TRUST_PROJECT_CONFIG=true`) is a **global** +> trust grant for the whole mcpls process — it is not scoped to a single project. +> Prefer setting it on a per-project MCP client config entry (the `args`/`env` for +> that project's `mcpls` server registration) rather than in your shell profile or +> a user-global MCP client config, so it doesn't silently apply the next time +> mcpls is launched against a different, untrusted checkout. +> +> `$MCPLS_CONFIG` is a second, by-design door past this gate: it is always +> trusted regardless of this flag, including when set to a relative path. A +> repository's own `.envrc` (or similar) exporting `MCPLS_CONFIG=./mcpls.toml` +> would make direnv-style tooling load it automatically — not a bug (an +> explicitly named path is consent, per the design above), but worth knowing if +> you audit a checkout for auto-executing config before running mcpls in it. + ## Configuration Structure ```toml diff --git a/docs/user-guide/getting-started.md b/docs/user-guide/getting-started.md index f78c7668..c1d6433b 100644 --- a/docs/user-guide/getting-started.md +++ b/docs/user-guide/getting-started.md @@ -90,7 +90,8 @@ mcpls works zero-config for Rust projects (uses rust-analyzer by default). For o mcpls searches for configuration in: 1. Path specified by `--config` flag 2. `$MCPLS_CONFIG` environment variable -3. `./mcpls.toml` (current directory) +3. `./mcpls.toml` (current directory) — requires `--trust-project-config` (see + [Configuration Reference](configuration.md#trusting-a-project-local-config)) 4. `~/.config/mcpls/mcpls.toml` ### Example Configuration diff --git a/docs/user-guide/installation.md b/docs/user-guide/installation.md index 52977226..1c202697 100644 --- a/docs/user-guide/installation.md +++ b/docs/user-guide/installation.md @@ -294,7 +294,8 @@ mcpls searches for configuration files in the following order: 1. Path specified by `--config` flag 2. `$MCPLS_CONFIG` environment variable -3. `./mcpls.toml` (current directory) +3. `./mcpls.toml` (current directory) — requires `--trust-project-config` (see + [Configuration Reference](configuration.md#trusting-a-project-local-config)) 4. `~/.config/mcpls/mcpls.toml` (user config directory) ### Minimal Configuration diff --git a/docs/user-guide/troubleshooting.md b/docs/user-guide/troubleshooting.md index ee49fc42..64f651c2 100644 --- a/docs/user-guide/troubleshooting.md +++ b/docs/user-guide/troubleshooting.md @@ -353,6 +353,13 @@ mkdir -p ~/.config/mcpls cp mcpls.toml ~/.config/mcpls/ ``` +**Solution 4**: If `mcpls.toml` is in the current directory, it is ignored by +default and a warning is logged (`mcpls --log-level warn` shows it). Opt in +explicitly: +```bash +mcpls --trust-project-config +``` + ### "Invalid configuration: missing field" **Problem**: TOML syntax error or missing required field diff --git a/examples/mcpls.toml b/examples/mcpls.toml index a45993d4..7ce3408b 100644 --- a/examples/mcpls.toml +++ b/examples/mcpls.toml @@ -1,5 +1,8 @@ # Example MCPLS configuration file -# Copy to ~/.config/mcpls/mcpls.toml or specify with --config flag +# Copy to ~/.config/mcpls/mcpls.toml or specify with --config flag. +# If placed as ./mcpls.toml in a project directory instead, mcpls ignores it +# unless you pass --trust-project-config (or set MCPLS_TRUST_PROJECT_CONFIG=true) — +# see docs/user-guide/configuration.md#trusting-a-project-local-config. # Workspace settings [workspace]