Skip to content

fix(core): encode reserved path chars in file URIs #151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dergachoff wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(core): encode reserved path chars in file URIs #151

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 58 additions & 9 deletions crates/mcpls-core/src/bridge/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,19 +212,46 @@ impl DocumentTracker {
/// not occur for valid absolute paths.
#[must_use]
pub fn path_to_uri(path: &Path) -> Uri {
let uri_string = if cfg!(windows) {
let path_str = path.to_string_lossy();
// canonicalize() on Windows adds a \\?\ extended-path prefix.
// Strip it before building the URI — file:////?\C:/ is not valid.
let stripped = path_str.strip_prefix(r"\\?\").unwrap_or(&path_str);
format!("file:///{}", stripped.replace('\\', "/"))
} else {
format!("file://{}", path.display())
};
let uri_string = file_uri_string(path);
let uri_string = encode_rfc3986_path_chars(&uri_string);
#[allow(clippy::expect_used)]
uri_string.parse().expect("failed to create URI from path")
}

#[cfg(not(windows))]
fn file_uri_string(path: &Path) -> String {
#[allow(clippy::expect_used)]
let file_url = Url::from_file_path(path).expect("failed to create file URI from path");
file_url.into()
}

#[cfg(windows)]
fn file_uri_string(path: &Path) -> String {
match Url::from_file_path(path) {
Ok(file_url) => file_url.into(),
Err(()) if path.has_root() => windows_rooted_path_to_file_uri(path),
Err(()) => panic!("failed to create file URI from path"),
}
}

#[cfg(windows)]
fn windows_rooted_path_to_file_uri(path: &Path) -> String {
let path_str = path.to_string_lossy();
let stripped = path_str.strip_prefix(r"\\?\").unwrap_or(&path_str);
format!("file:///{}", stripped.replace('\\', "/"))
}

fn encode_rfc3986_path_chars(uri: &str) -> String {
let path_start = url::Position::BeforePath as usize;

@bug-ops bug-ops May 26, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[C1 — critical] url::Position::BeforePath is an enum discriminant (10), not a byte offset in the URI string. For file:///[a].ts the [ sits at byte 8 — it falls in the "prefix" slice and is never encoded. The stated motivation for this fix (Next.js [slug] files) silently fails.

Use the Index trait that the url crate actually provides for this:

fn encode_rfc3986_path_chars(uri: &str) -> String {
    let url = Url::parse(uri).expect("encode called with invalid URI");
    let prefix = url[..url::Position::BeforePath].to_owned();
    let encoded = url[url::Position::BeforePath..]
        .replace('[', "%5B")
        .replace(']', "%5D")
        .replace('^', "%5E")
        .replace('|', "%7C");
    format!("{prefix}{encoded}")
}

let (prefix, path) = uri.split_at(path_start);
let encoded_path = path

@bug-ops bug-ops May 26, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[M2 — minor] {, }, and backtick are also RFC 3986 §2.2 reserved characters omitted here. Low risk for WHATWG-compliant consumers (rust-analyzer, pyright), but silently wrong for strict RFC 3986 clients. Tracking in a follow-up issue.

.replace('[', "%5B")
.replace(']', "%5D")
.replace('^', "%5E")
.replace('|', "%7C");
format!("{prefix}{encoded_path}")
}

/// Convert an LSP `file://` URI to an absolute filesystem path.
///
/// Returns `None` if the URI is not a valid `file://` URI, uses a non-file
Expand Down Expand Up @@ -636,6 +663,28 @@ mod tests {
assert!(uri.as_str().contains("project-test"));
}

#[test]
fn test_path_to_uri_percent_encodes_reserved_chars() {
#[cfg(windows)]
let path = Path::new(r"C:\home\user\routes\api\[...]^|.ts");
#[cfg(not(windows))]
let path = Path::new("/home/user/routes/api/[...]^|.ts");

@bug-ops bug-ops May 26, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[M1 — minor] Only a long path is tested; the split_at(10) bug does not surface here. Add a short case:

let path = Path::new("/[a].ts");
let uri = path_to_uri(path);
assert!(uri.as_str().ends_with("%5Ba%5D.ts"));


let uri = path_to_uri(path);

#[cfg(windows)]
let expected = "file:///C:/home/user/routes/api/%5B...%5D%5E%7C.ts";
#[cfg(not(windows))]
let expected = "file:///home/user/routes/api/%5B...%5D%5E%7C.ts";

assert_eq!(uri.as_str(), expected);
assert_eq!(
uri_to_path(&uri).as_deref(),
Some(path),
"encoded file URI should round-trip to the original path"
);
}

#[test]
fn test_document_tracker_concurrent_operations() {
let mut map = HashMap::new();
Expand Down
Loading