fix: gate untrusted project-local mcpls.toml and scope document_tracker lock per path#245
Merged
Merged
Conversation
The document_tracker lock spanned every ensure_open await -- a disk stat, an optional full re-read, and the didOpen/didChange notify (a bounded channel send with no timeout) -- across every language and path. A wedged language server that stopped draining its stdin could stall ensure_open for unrelated files and languages, not just the one talking to that server. DocumentTracker now guards its map only for short synchronous sections and serializes ensure_open per path via a keyed async lock, so concurrent opens on different paths no longer wait on each other while concurrent opens on the same path still collapse into exactly one didOpen. Fixes #227
mcpls auto-loaded ./mcpls.toml relative to the process working directory and fed its command/args/[workspace] straight into the spawned LSP server with no confirmation. Since mcpls is normally started against an arbitrary checkout by an AI agent, a hostile repository could ship an mcpls.toml that spawns an attacker-chosen process the moment mcpls starts -- effectively code execution via git clone && start mcpls in that directory. Interactive confirmation isn't viable since stdio is the MCP transport itself, so the gate is opt-in and load-time: a CWD-discovered ./mcpls.toml is now ignored by default (logging a warning that names the ignored path), falling through to global config and the existing built-in project-marker heuristics. Passing --trust-project-config or setting MCPLS_TRUST_PROJECT_CONFIG=true opts back in. An explicit --config/MCPLS_CONFIG path is unaffected, since naming a path is itself consent. Note: the original report's claim that env is attacker-controlled at spawn time doesn't hold today -- env is parsed but never passed to the spawned process. This fix is scoped to command/args/ [workspace]. Fixes #229
There was a problem hiding this comment.
Pull request overview
This PR hardens mcpls startup against untrusted workspace configuration by gating CWD-discovered ./mcpls.toml, and improves concurrency in the MCP↔LSP bridge by scoping document tracking synchronization to a per-path async lock instead of a single global lock.
Changes:
- Add an explicit trust gate (
--trust-project-config/MCPLS_TRUST_PROJECT_CONFIG) so./mcpls.tomlin the current directory is ignored by default unless opted in. - Refactor
DocumentTrackerto avoid holding a global lock across disk I/O / LSP notifications by using per-path async locking and short-lived std mutex critical sections. - Update docs, changelog, and tests (including CLI integration and concurrency regressions) to cover the new behavior and API changes.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents MCPLS_TRUST_PROJECT_CONFIG and warns that ./mcpls.toml is ignored by default. |
| examples/mcpls.toml | Notes the new trust gate behavior for project-local configs. |
| docs/user-guide/troubleshooting.md | Adds guidance for the “CWD config ignored” warning and how to opt in. |
| docs/user-guide/installation.md | Updates config discovery order to reflect the trust requirement. |
| docs/user-guide/getting-started.md | Updates config discovery order to reflect the trust requirement. |
| docs/user-guide/configuration.md | Adds a dedicated “Trusting a Project-Local Config” section and clarifies trust semantics. |
| crates/mcpls-core/tests/integration/basic_tests.rs | Adjusts tests to account for Translator APIs no longer being async. |
| crates/mcpls-core/src/mcp/server.rs | Updates resource listing to use non-async Translator::open_document_paths. |
| crates/mcpls-core/src/lib.rs | Re-exports ProjectConfigTrust as part of the public API surface. |
| crates/mcpls-core/src/config/mod.rs | Introduces ProjectConfigTrust and ServerConfig::load_with_trust to gate CWD config loading; adds tests. |
| crates/mcpls-core/src/bridge/translator.rs | Removes outer async lock around DocumentTracker; relies on internal per-path locking. |
| crates/mcpls-core/src/bridge/state.rs | Implements per-path async locking + short std-mutex map locks; adds concurrency regression tests. |
| crates/mcpls-core/src/bridge/mod.rs | Centralizes lock_std helper for consistent poisoned-mutex recovery. |
| crates/mcpls-cli/tests/cli_integration.rs | Adds CLI regression tests for trust gating; ensures ambient env doesn’t leak into tests. |
| crates/mcpls-cli/src/main.rs | Wires CLI flag/env into ServerConfig::load_with_trust. |
| crates/mcpls-cli/src/args.rs | Adds --trust-project-config / MCPLS_TRUST_PROJECT_CONFIG argument and parsing tests. |
| crates/mcpls-cli/README.md | Documents the new trust gate and CLI flag/env var. |
| CHANGELOG.md | Adds entries for the new trust gate and document tracker changes (needs minor structure adjustment per guideline). |
bug-ops
enabled auto-merge (squash)
July 25, 2026 02:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent fixes, developed and reviewed together since they landed in the same worktree/branch.
#229 (P1, security) — untrusted project-local config could redirect the spawned LSP command
ServerConfig::load()discovered and loaded./mcpls.tomlrelative to the process's current working directory unconditionally, and that file'scommand/args/[workspace]fed directly into the LSP server mcpls spawns. Running mcpls against an untrusted checkout (git clone && mcpls) could execute an attacker-chosen command with no confirmation.Interactive confirmation isn't viable since stdio is the MCP transport itself, so the fix is a load-time opt-in gate: a CWD-discovered
./mcpls.tomlis now ignored by default (with a warning naming the ignored path), falling through to global config and the existing built-in project-marker heuristics. Pass--trust-project-configor setMCPLS_TRUST_PROJECT_CONFIG=trueto opt in. An explicit--config/MCPLS_CONFIGpath is unaffected — naming a path is itself consent.Note: the original report's claim that
envis attacker-controlled at spawn time does not hold today —envis parsed but never passed to the spawned process. This fix is scoped tocommand/args/[workspace].Two informational findings surfaced during audit are explicitly out of scope for this PR and worth their own issues:
envfield is parsed but never applied to the spawned process (dead field today, so not currently exploitable).env_clear()before spawn — a trusted server's child process inherits mcpls's full environment.ServerInfo.instructionsin a follow-up.Breaking change (acceptable pre-1.0): existing users relying on bare
./mcpls.tomlauto-load must now pass--trust-project-config/ set the env var.#227 (P3) — document_tracker lock spanned disk I/O and didOpen notify in ensure_open
The
document_trackerlock was a single lock shared across every language and path, held acrossensure_open's own awaits (a disk stat, an optional full file re-read, and thetextDocument/didOpennotify — a bounded channel send with no timeout). A wedged language server that stopped draining its stdin could stallensure_openfor unrelated files and languages.DocumentTrackernow guards its map only for short synchronous sections and serializesensure_openper path via a keyed async lock: concurrent opens on different paths no longer wait on each other, while concurrent opens on the same path still collapse into exactly onedidOpen.Breaking change (acceptable pre-1.0):
DocumentTrackermethods now take&selfinstead of&mut self;getreturns an ownedDocumentState;open_pathsreturnsVec<PathBuf>;Translator::open_document_paths/is_document_openare no longerasync.Test plan
cargo +nightly fmt --all -- --checkcargo clippy --all-targets --all-features --workspace -- -D warningscargo nextest run --workspace --all-features --lib --bins(455 passed)RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-featuresdidOpen) and CLI subprocess tests for the trust gate (flag grant, envtruegrant, envfalse/malformed no-grant)mcpls.toml(untrusted run does not spawn the hostile command;--trust-project-configdoes)Closes #229, closes #227