Confine MCP file resources to the workspace - #629
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Advertised file:/// URIs use four leading slashes on Unix; round-trip through Url::to_file_path is untested
handle_workspace_files advertises resource URIs as format!("file:///{}", path.replace("\\", "/")) (wfl-lsp/src/mcp_server.rs:916). On Unix, entry.path() is an absolute path beginning with / (e.g. /ws/program.wfl), so the emitted URI becomes file:////ws/program.wfl (four slashes), which is not the canonical serialization url produces (Url::from_file_path yields three slashes). The new reader handle_file_resource (wfl-lsp/src/mcp_server.rs:995) now parses the URI via Url::parse(...).to_file_path() instead of the old strip_prefix("file:///"). The old strip-prefix approach round-tripped the four-slash form fine (yielding /ws/program.wfl). Whether Url::to_file_path() returns a usable path for the non-canonical four-slash form (empty-host file URL with a leading empty path segment) is not exercised by any test — all new tests build URIs with Url::from_file_path (the canonical three-slash form). If to_file_path() fails or returns a path that then fails canonicalize/starts_with, clients that read a URI verbatim from workspace://files could get 'Invalid local file resource URI' or 'does not exist' errors even for legitimate in-workspace files. Worth confirming the round-trip against the pinned url crate version, or normalizing the advertised URI via Url::from_file_path.
(Refers to line 916)
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2332593361
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| data: None, | ||
| }), | ||
| }, | ||
| let requested_path = match Url::parse(uri).ok().and_then(|url| url.to_file_path().ok()) { |
There was a problem hiding this comment.
When a workspace file name contains URL-reserved characters such as # or ?, workspace://files still emits the raw path (for example file:////.../foo#bar.wfl). With the new Url::parse(...).to_file_path() here, those characters are interpreted as fragment/query delimiters before canonicalization, so a client that lists resources and then reads the returned URI gets File resource does not exist even though the .wfl file is inside the workspace. Please emit/read proper file URLs, e.g. by using Url::from_file_path, so listed resources round-trip.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens the WFL MCP server’s resources/read handling to ensure file:/// resources and .wflcfg reads are confined to the configured workspace, preventing traversal/symlink escapes and limiting read sizes to reduce data exposure and memory risk.
Changes:
- Canonicalizes workspace root + requested
file:///target, rejects out-of-workspace/symlink escapes, and restricts readable resources to.wflfiles. - Adds a bounded text reader (4 MiB cap) and applies it to file resources, symbol scanning, diagnostics scanning, and workspace config reads.
- Stops echoing full MCP request/response payloads to stderr and adds regression tests for workspace containment and size limits.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| wfl-lsp/src/mcp_server.rs | Adds canonicalization/containment enforcement, 4 MiB bounded reads, quieter logging, and new regression tests for MCP resource/config reads. |
| CHANGELOG.md | Documents the new MCP workspace containment and logging changes under Security. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let requested_path = match Url::parse(uri).ok().and_then(|url| url.to_file_path().ok()) { | ||
| Some(path) => path, | ||
| None => return Self::file_resource_error(id, "Invalid local file resource URI"), | ||
| }; |
| fn file_resource_error(id: Option<Value>, message: impl Into<String>) -> JsonRpcResponse { | ||
| JsonRpcResponse { | ||
| jsonrpc: "2.0".to_string(), | ||
| id, | ||
| result: None, | ||
| error: Some(JsonRpcError { | ||
| code: -32602, | ||
| message: message.into(), | ||
| data: None, | ||
| }), | ||
| } |
| let requested_path = match Url::parse(uri).ok().and_then(|url| url.to_file_path().ok()) { | ||
| Some(path) => path, | ||
| None => return Self::file_resource_error(id, "Invalid local file resource URI"), | ||
| }; | ||
| let requested_path = match requested_path.canonicalize() { |
| let metadata = path | ||
| .metadata() | ||
| .map_err(|_| "File resource is not readable".to_string())?; | ||
| if !metadata.is_file() { | ||
| return Err("File resource is not a regular file".to_string()); | ||
| } |
| fs::File::open(path) | ||
| .and_then(|file| { | ||
| file.take(MAX_MCP_RESOURCE_BYTES + 1) | ||
| .read_to_end(&mut bytes) | ||
| }) | ||
| .map_err(|_| "Failed to read file resource".to_string())?; | ||
| if bytes.len() as u64 > MAX_MCP_RESOURCE_BYTES { | ||
| return Err(format!( | ||
| "File resource exceeds the {} byte limit", | ||
| MAX_MCP_RESOURCE_BYTES | ||
| )); | ||
| } | ||
| String::from_utf8(bytes).map_err(|_| "File resource is not valid UTF-8".to_string()) |
|
Superseded by #632, which preserves this security fix in the consolidated Rust-source hardening PR. The combined head is mergeable and all required CI checks are green. |
Summary
file:///resources as real file URLs and canonicalize both the target and configured workspace.wflsource files and cap each resource/config/symbol read at 4 MiB.wflcfgSecurity impact
A caller of the local MCP
resources/readmethod could previously select paths outside the workspace and make the server buffer them without a source-level limit. Malicious workspace symlinks could also redirectworkspace://configto an external file. The server now returns only bounded, canonical workspace-owned source/config content and no longer copies source payloads into logs.Validation
git diff --checkpassesPart of the Rust-source security audit tracked in #610.