diff --git a/Cargo.lock b/Cargo.lock index fbb88db16..661bd9d94 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4964,9 +4964,9 @@ dependencies = [ name = "ide-web" version = "0.1.42" dependencies = [ + "dirs 6.0.0", "ide-core", "open", - "parking_lot", "percent-encoding", "serde", "specta", diff --git a/crates/ide-agent/src/worktree.rs b/crates/ide-agent/src/worktree.rs index 6ee370959..aec980a76 100644 --- a/crates/ide-agent/src/worktree.rs +++ b/crates/ide-agent/src/worktree.rs @@ -14,8 +14,18 @@ pub(crate) struct Created { pub base_commit: String, } +/// The repository of `path`, free to execute: the host runs every agent command that +/// reaches a repository only in a trusted project. +fn discover(path: &Path) -> IdeResult> { + let repo = Repo::discover(path)?; + if let Some(repo) = &repo { + repo.set_execution_allowed(true); + } + Ok(repo) +} + fn repo_at(path: &Path) -> IdeResult { - Repo::discover(path)?.ok_or_else(|| IdeError::unavailable(format!("{} is not in a git repository", path.display()))) + discover(path)?.ok_or_else(|| IdeError::unavailable(format!("{} is not in a git repository", path.display()))) } /// The current branch (or commit when detached) and its commit. @@ -91,7 +101,7 @@ fn retrying(attempts: u32, pause: Duration, mut op: impl FnMut() -> std::io::Res /// Files under `cwd` that differ from `base` (untracked files included), with line counts. /// Outside a repository there is nothing to compare against: no changes. pub(crate) fn changes(cwd: &Path, base: &str) -> IdeResult> { - let Some(repo) = Repo::discover(cwd)? else { return Ok(Vec::new()) }; + let Some(repo) = discover(cwd)? else { return Ok(Vec::new()) }; let root = repo.root().to_path_buf(); let stats: std::collections::HashMap = repo.numstat_with_worktree(base)?.into_iter().map(|(p, a, r)| (p, (a, r))).collect(); @@ -209,7 +219,7 @@ pub(crate) fn restore( } pub(crate) fn file_at(cwd: &Path, path: &Path, rev: &str) -> IdeResult> { - let Some(repo) = Repo::discover(cwd)? else { return Ok(None) }; + let Some(repo) = discover(cwd)? else { return Ok(None) }; let encoding = match ide_vfs::load_text(path) { Ok(loaded) => encoding_rs::Encoding::for_label(loaded.encoding.as_bytes()).unwrap_or(encoding_rs::UTF_8), Err(_) => ide_vfs::detect_encoding(&repo.file_bytes_at(path, Some(rev))?.unwrap_or_default(), None), diff --git a/crates/ide-extension/src/capability.rs b/crates/ide-extension/src/capability.rs index c17930adb..efbc3a395 100644 --- a/crates/ide-extension/src/capability.rs +++ b/crates/ide-extension/src/capability.rs @@ -71,7 +71,6 @@ impl Capability { match self { Self::ProjectRead | Self::EditorRead | Self::Storage | Self::Events | Self::VcsRead => Risk::Low, Self::EditorWrite - | Self::ProjectWrite | Self::DatabaseSources | Self::VcsWrite | Self::Net(_) @@ -80,7 +79,14 @@ impl Capability { | Self::Secrets | Self::InputEvents | Self::Clipboard => Risk::Medium, - Self::IdeCommands | Self::Ipc(_) | Self::Process(_) | Self::FsRead(_) | Self::FsWrite(_) => Risk::High, + // A trusted project's own files name the programs the IDE starts (its settings, its + // build scripts), so writing them is running code. + Self::ProjectWrite + | Self::IdeCommands + | Self::Ipc(_) + | Self::Process(_) + | Self::FsRead(_) + | Self::FsWrite(_) => Risk::High, } } @@ -402,6 +408,15 @@ mod tests { assert!(Capability::Storage.auto_migrates() && !Capability::EditorWrite.auto_migrates()); } + /// A trusted project's files name programs the IDE starts by itself: the git and language + /// server executables in `.openidea/settings.toml`, its build scripts. Writing them is + /// running code, and the install dialog says so. + #[test] + fn writing_project_files_is_a_high_risk_grant() { + assert_eq!(Capability::ProjectWrite.risk(), Risk::High); + assert!(!Capability::ProjectWrite.auto_migrates()); + } + #[test] fn host_patterns() { assert!(host_matches("crates.io", "crates.io")); diff --git a/crates/ide-extension/src/package.rs b/crates/ide-extension/src/package.rs index 43845c5da..37ed1faaf 100644 --- a/crates/ide-extension/src/package.rs +++ b/crates/ide-extension/src/package.rs @@ -405,10 +405,31 @@ impl Index { /// revocations, publisher keys (revoked ones) and `moved` — and lists no /// plugins ([`Index::is_newer`]): the IDE needs updating to install from it. pub fn parse(text: &str) -> Result { + let (index, dropped) = Self::parse_dropping(text)?; + if let Some(first) = dropped.first() { + // One line per index, and a bounded one: whoever wrote the index chose the text. + let id: String = first.id.chars().take(80).collect(); + tracing::warn!( + "index: {} entries are not offered, their id or version is not one a manifest may carry (first: {id:?})", + dropped.len() + ); + } + Ok(index) + } + + /// [`Index::parse`], with the entries it does not offer: those whose id or + /// version no manifest may carry, so no IDE could install them. + pub fn parse_dropping(text: &str) -> Result<(Self, Vec), String> { let value: serde_json::Value = serde_json::from_str(text).map_err(|e| format!("index: {e}"))?; let format = value.get("format").and_then(serde_json::Value::as_u64).ok_or("index: `format` is missing")?; if format == u64::from(INDEX_FORMAT) { - return serde_json::from_value(value).map_err(|e| format!("index: {e}")); + let mut index: Index = serde_json::from_value(value).map_err(|e| format!("index: {e}"))?; + // What no manifest may carry is not offered: the id and version also name files on disk. + let (offered, dropped) = std::mem::take(&mut index.plugins) + .into_iter() + .partition(|e| crate::validate_plugin_id(&e.id).is_ok() && e.semver().is_some()); + index.plugins = offered; + return Ok((index, dropped)); } if format < u64::from(INDEX_FORMAT) { return Err(format!("index format {format} is not supported (this IDE reads {INDEX_FORMAT})")); @@ -417,13 +438,14 @@ impl Index { let items = value.get(field).and_then(serde_json::Value::as_array); items.into_iter().flatten().filter_map(|v| serde_json::from_value(v.clone()).ok()).collect() } - Ok(Index { + let index = Index { format: u32::try_from(format).unwrap_or(u32::MAX), publishers: each(&value, "publishers"), plugins: Vec::new(), revoked: each(&value, "revoked"), moved: value.get("moved").and_then(serde_json::Value::as_str).map(str::to_owned), - }) + }; + Ok((index, Vec::new())) } /// Written in a format newer than this IDE reads: it lists no plugins. @@ -908,6 +930,51 @@ mod tests { } } + #[test] + fn an_index_offers_only_the_ids_and_versions_a_manifest_may_carry() { + let invalid_ids = [ + "../x", + "../../escape", + "acme/../x", + r"acme.\..\x", + "acme./../../../escape", + "/abs", + r"C:\x", + "ACME.x", + "acme.", + "acme..x", + "acme.x y", + "", + ]; + let invalid_versions = + ["../1.0.0", "1.0.0/../../../escape", r"1.0.0\..\x", "1.0.0-..", " 1.0.0", "1.0", "latest", ""]; + let valid = [("acme.x", "1.0.0"), ("acme.tools-2_b", "1.0.0-rc.1+b.2")]; + let mut listed: Vec<(&str, &str)> = valid.to_vec(); + listed.extend(invalid_ids.iter().map(|id| (*id, "1.0.0"))); + listed.extend(invalid_versions.iter().map(|version| ("acme.x", *version))); + let plugins: Vec = listed + .iter() + .map(|(id, version)| { + serde_json::json!({ + "id": id, "name": "X", "version": version, "publisher": "acme", + "download": "x.oiplugin", "digest": "d", "engine": "^1.0", + }) + }) + .collect(); + let text = serde_json::json!({ "format": INDEX_FORMAT, "plugins": plugins }).to_string(); + let pairs = |entries: &[IndexEntry]| -> Vec<(String, String)> { + entries.iter().map(|e| (e.id.clone(), e.version.clone())).collect() + }; + let owned = |pairs: &[(&str, &str)]| -> Vec<(String, String)> { + pairs.iter().map(|(id, version)| ((*id).to_owned(), (*version).to_owned())).collect() + }; + + assert_eq!(pairs(&Index::parse(&text).unwrap().plugins), owned(&valid)); + let (index, dropped) = Index::parse_dropping(&text).unwrap(); + assert_eq!(pairs(&index.plugins), owned(&valid)); + assert_eq!(pairs(&dropped), owned(&listed[valid.len()..])); + } + #[test] fn a_newer_index_still_revokes_and_moves() { let newer = format!( diff --git a/crates/ide-github/src/lib.rs b/crates/ide-github/src/lib.rs index 720490833..6004a41df 100644 --- a/crates/ide-github/src/lib.rs +++ b/crates/ide-github/src/lib.rs @@ -102,6 +102,14 @@ fn parse(r: serde_json::Result) -> IdeResult { r.map_err(|e| IdeError::new(ide_core::error::ErrorKind::Protocol, format!("unexpected gh output: {e}"))) } +/// The repository of `root`, free to execute: the host runs the `gh_*` commands only in a +/// trusted project. +fn trusted_repo(root: &Path) -> IdeResult { + let repo = Repo::discover(root)?.ok_or_else(|| IdeError::invalid("not a git repository"))?; + repo.set_execution_allowed(true); + Ok(repo) +} + /// Whether gh is installed and signed in, and which GitHub repository `root` maps to. pub fn availability(root: &Path) -> GhAvailability { if gh_executable().is_none() { @@ -233,7 +241,7 @@ pub fn create( return Err(IdeError::invalid("choose a base branch")); } if req.push { - let repo = Repo::discover(root)?.ok_or_else(|| IdeError::invalid("not a git repository"))?; + let repo = trusted_repo(root)?; repo.push(false, true, cancel, on_output)?; } let mut args = vec!["pr", "create", "--title", req.title.trim(), "--body", &req.body, "--base", req.base.trim()]; @@ -354,7 +362,7 @@ pub fn prepare_diff( cancel: &CancellationToken, on_output: &dyn Fn(String), ) -> IdeResult { - let repo = Repo::discover(root)?.ok_or_else(|| IdeError::invalid("not a git repository"))?; + let repo = trusted_repo(root)?; if !repo.has_commit(head_oid) || !repo.has_commit(base_oid) { let remotes = repo.remotes()?; let remote = remote_for(&remotes, name_with_owner) diff --git a/crates/ide-host/src/bin/openidea-plugin/main.rs b/crates/ide-host/src/bin/openidea-plugin/main.rs index ca9ccdd58..64d335117 100644 --- a/crates/ide-host/src/bin/openidea-plugin/main.rs +++ b/crates/ide-host/src/bin/openidea-plugin/main.rs @@ -22,7 +22,7 @@ mod templates; use ide_extension::{Manifest, validate_plugin_id}; -use ide_host::plugins::{PluginHost, plugin_icon, read_market_url, resolve_market_href}; +use ide_host::plugins::{PluginHost, market_href_reachable_from, plugin_icon, read_market_url, resolve_market_href}; use ide_plugin::{Limits, PluginEngine}; use std::path::{Path, PathBuf}; use std::process::{Command, ExitCode}; @@ -591,7 +591,7 @@ fn cmd_index(args: &[String]) -> Result<(), String> { let base = a.value("download-base").map(|b| b.trim_end_matches('/')); // Revocations and other publishers of an existing index are kept. let mut index = match std::fs::read_to_string(&out) { - Ok(text) => current_format(Index::parse(&text)?, &out.to_string_lossy())?, + Ok(text) => parse_index(&text, &out.to_string_lossy())?, Err(_) => Index { format: package::INDEX_FORMAT, ..Index::default() }, }; let publisher = match (a.value("publisher"), a.value("key")) { @@ -722,14 +722,22 @@ fn register_key( Ok(publishers) } -/// `index`, unless a newer `openidea-plugin` wrote it (rewriting it would drop what this one can't read). -fn current_format(index: Index, file: &str) -> Result { +/// The index in `text` (read from `file`), unless a newer `openidea-plugin` wrote it (rewriting it +/// would drop what this one can't read). Says which entries it does not keep: no IDE offers them. +fn parse_index(text: &str, file: &str) -> Result { + let (index, dropped) = Index::parse_dropping(text)?; if index.is_newer() { return Err(format!( "{file}: index format {} is newer than this tool writes; update openidea-plugin", index.format )); } + for e in &dropped { + eprintln!( + "warning: {file}: dropped {:?} {:?}: its id or version is not one a manifest may carry", + e.id, e.version + ); + } Ok(index) } @@ -740,14 +748,15 @@ fn cmd_merge(args: &[String]) -> Result<(), String> { }; let keep = a.value("keep").ok_or("--keep is required")?; let out = a.value("out").unwrap_or(base); - let mut index = - current_format(Index::parse(&std::fs::read_to_string(base).map_err(|e| format!("{base}: {e}"))?)?, base)?; + let mut index = parse_index(&std::fs::read_to_string(base).map_err(|e| format!("{base}: {e}"))?, base)?; let bytes = read_market_url(other, 16 << 20)?; - let theirs = Index::parse(&String::from_utf8(bytes).map_err(|_| format!("{other}: not UTF-8"))?)?; - let mut theirs = current_format(theirs, other)?; + let mut theirs = parse_index(&String::from_utf8(bytes).map_err(|_| format!("{other}: not UTF-8"))?, other)?; // Their downloads are relative to their index, not to ours. for e in &mut theirs.plugins { e.download = resolve_market_href(other, &e.download); + if !market_href_reachable_from(other, &e.download) { + return Err(format!("{other}: the download of {} {} is not on the network", e.id, e.version)); + } } let count = theirs.plugins.len(); index.include(theirs, &[keep]).map_err(|e| format!("{other}: {e}"))?; @@ -947,6 +956,47 @@ mod tests { assert!(err.contains("unknown publisher"), "{err}"); } + /// An index read over the network is not merged when it names a download on this machine: + /// the merged index is a local one, which may. + #[test] + fn an_index_read_over_the_network_is_not_merged_with_local_downloads() { + use std::io::{BufRead, BufReader, Write}; + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path().join("index.json"); + write_index(&Index { format: package::INDEX_FORMAT, ..Index::default() }, &base).unwrap(); + let local = tmp.path().join("alice.dawn-1.0.0.oiplugin").to_string_lossy().into_owned(); + let theirs = serde_json::json!({ + "format": package::INDEX_FORMAT, + "publishers": [{ "id": "alice", "name": "Alice", "keys": [] }], + "plugins": [{ + "id": "alice.dawn", "name": "Dawn", "version": "1.0.0", "publisher": "alice", + "download": local, "digest": "d", "engine": "^1.0", + }], + }) + .to_string(); + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let other = format!("http://127.0.0.1:{}/index.json", listener.local_addr().unwrap().port()); + std::thread::spawn(move || { + for mut stream in listener.incoming().flatten() { + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut line = String::new(); + while reader.read_line(&mut line).unwrap_or(0) > 0 && line != "\r\n" { + line.clear(); + } + let _ = write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{theirs}", + theirs.len() + ); + } + }); + + let err = cmd_merge(&strings(&[base.to_str().unwrap(), &other, "--keep", "openidea"])).unwrap_err(); + assert!(err.contains("is not on the network"), "{err}"); + let merged = Index::parse(&std::fs::read_to_string(&base).unwrap()).unwrap(); + assert!(merged.plugins.is_empty(), "{:?}", merged.plugins); + } + #[test] fn the_index_carries_plugin_icons() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/ide-host/src/mcp/vcs.rs b/crates/ide-host/src/mcp/vcs.rs index 92f784d80..359edd8f2 100644 --- a/crates/ide-host/src/mcp/vcs.rs +++ b/crates/ide-host/src/mcp/vcs.rs @@ -45,26 +45,12 @@ fn innermost_repo(repos: Vec<(PathBuf, T)>, path: &Path) -> Option<(PathBuf, // Read tools must never invoke repository-configured executables, including in Restricted Mode. fn read_command(git: &str, root: &Path) -> std::process::Command { let mut command = std::process::Command::new(git); + ide_vcs::restrict(&mut command); command .current_dir(root) - .args([ - "-c", - "core.fsmonitor=false", - "-c", - "core.hooksPath=", - "-c", - "diff.external=", - "-c", - "core.attributesFile=", - "-c", - "submodule.recurse=false", - "-c", - "color.ui=false", - ]) - .env("GIT_NO_LAZY_FETCH", "1") + .args(["-c", "color.ui=false"]) .env("GIT_TERMINAL_PROMPT", "0") .env("GIT_OPTIONAL_LOCKS", "0") - .env_remove("GIT_EXTERNAL_DIFF") .env_remove("GIT_DIR") .env_remove("GIT_WORK_TREE") .env_remove("GIT_INDEX_FILE"); @@ -72,6 +58,32 @@ fn read_command(git: &str, root: &Path) -> std::process::Command { command } +/// `-c` arguments that empty the clean and process filters the repository's own config names: +/// git runs them to compare a changed file with the index. Filters of the user's config stay +/// (Git LFS), as they are not the repository's choice. +fn filter_overrides(git: &str, root: &Path) -> IdeResult> { + let mut cmd = read_command(git, root); + cmd.args(["config", "--null", "--list", "--show-scope"]); + let out = cmd.output().map_err(|e| IdeError::unavailable(format!("cannot run git: {e}")))?; + if !out.status.success() { + return Err(IdeError::internal(String::from_utf8_lossy(&out.stderr).trim().to_owned())); + } + repository_filter_overrides(&out.stdout) +} + +/// [`filter_overrides`] out of `listing`, the output of `git config --null --list --show-scope`. +fn repository_filter_overrides(listing: &[u8]) -> IdeResult> { + let mut fields = listing.split(|b| *b == 0); + let mut own = Vec::new(); + while let (Some(scope), Some(record)) = (fields.next(), fields.next()) { + if matches!(scope, b"local" | b"worktree") { + own.extend_from_slice(record); + own.push(0); + } + } + ide_vcs::filter_overrides_from(&own) +} + /// The branch HEAD is on (unborn too); `None` when HEAD is detached or git fails. fn read_branch(git: &str, root: &Path) -> Option { let mut cmd = read_command(git, root); @@ -167,7 +179,10 @@ fn git_executable(call: &Call) -> String { /// `git status` of the repository at `root`, paths relative to it. fn read_status(git: &str, root: &Path, untracked: bool, ignored: bool) -> IdeResult> { let mut cmd = read_command(git, root); + cmd.args(filter_overrides(git, root)?); cmd.args(["-c", "core.quotepath=false", "status", "--porcelain", "-z", "--find-renames"]); + // git asks a submodule with changes for its own status, which runs under the submodule's config. + cmd.arg("--ignore-submodules=dirty"); // git refuses `--ignored=matching` with `--untracked-files=no`: untracked files are dropped below. cmd.arg(if untracked || ignored { "--untracked-files=all" } else { "--untracked-files=no" }); if ignored { @@ -183,6 +198,39 @@ fn read_status(git: &str, root: &Path, untracked: bool, ignored: bool) -> IdeRes Ok(entries) } +/// `git diff` of the repository at `root` (of `file` alone, when given): the index against HEAD +/// when `staged`, else disk against the index. +fn read_diff( + git: &str, + root: &Path, + file: Option<&Path>, + staged: bool, + cancel: &ide_core::CancellationToken, +) -> IdeResult { + let mut cmd = read_command(git, root); + cmd.args(filter_overrides(git, root)?); + cmd.args(["--no-pager", "diff", "--no-ext-diff", "--no-textconv"]); + // git asks a submodule with changes for its own status, and for its own diff when the + // repository's `diff.submodule` says so: both run under the submodule's config. + cmd.args(["--ignore-submodules=dirty", "--submodule=short"]); + if staged { + cmd.arg("--cached"); + } + cmd.arg("--"); + if let Some(file) = file { + cmd.arg(file.strip_prefix(root).map_err(|e| IdeError::invalid(e.to_string()))?); + } + ide_core::process::prepare(&mut cmd); + if cancel.is_cancelled() { + return Err(IdeError::cancelled()); + } + let out = cmd.output()?; + if !out.status.success() { + return Err(IdeError::unavailable(String::from_utf8_lossy(&out.stderr).trim())); + } + Ok(String::from_utf8_lossy(&out.stdout).into_owned()) +} + /// A git repository in the project, for paths outside the repositories IDEA knows: the /// closest folder with `.git` from `start` up to the project root. fn find_git_root(root: &Path, start: &Path) -> Option { @@ -307,18 +355,7 @@ fn git_diff() -> Tool { let staged = call.flag("staged", false)?; let git = git_executable(&call); let cancel = call.cancel.clone(); - let text = call.host.blocking(move || { - let mut cmd = read_command(&git, &root); - cmd.args(["--no-pager", "diff", "--no-ext-diff", "--no-textconv"]); - if staged { cmd.arg("--cached"); } - cmd.arg("--"); - if let Some(file) = file { cmd.arg(file.strip_prefix(&root).map_err(|e| IdeError::invalid(e.to_string()))?); } - ide_core::process::prepare(&mut cmd); - if cancel.is_cancelled() { return Err(IdeError::cancelled()); } - let out = cmd.output()?; - if !out.status.success() { return Err(IdeError::unavailable(String::from_utf8_lossy(&out.stderr).trim())); } - Ok(String::from_utf8_lossy(&out.stdout).into_owned()) - }).await?; + let text = call.host.blocking(move || read_diff(&git, &root, file.as_deref(), staged, &cancel)).await?; Ok(Output::text(text)) }) .param(Param::optional("repositoryPathRelativeToProject", ParamType::String, "Select the containing repository")) @@ -342,6 +379,252 @@ mod tests { assert!(e[3].conflicted() && !e[3].staged() && !e[3].unstaged()); } + /// Security audit lead 2 (variant): reading status or a diff must not run the repository's filters. + #[test] + fn status_and_diff_do_not_run_repository_filters() { + let dir = tempfile::tempdir().unwrap(); + let root = dunce::canonicalize(dir.path()).unwrap(); + let git = |args: &[&str]| { + let out = std::process::Command::new("git").current_dir(&root).args(args).output().unwrap(); + assert!(out.status.success(), "git {args:?}: {}", String::from_utf8_lossy(&out.stderr)); + }; + git(&["init", "-q"]); + git(&["config", "user.email", "t@example.test"]); + git(&["config", "user.name", "t"]); + git(&["config", "commit.gpgsign", "false"]); + std::fs::write(root.join(".gitattributes"), "*.txt filter=marker\n").unwrap(); + std::fs::write(root.join("a.txt"), "one\n").unwrap(); + git(&["add", "."]); + git(&["commit", "-qm", "init"]); + git(&["config", "filter.marker.clean", "echo x > executed-marker; cat"]); + git(&["config", "filter.marker.process", "echo x > executed-marker; cat"]); + git(&["config", "filter.marker.required", "true"]); + let marker = root.join("executed-marker"); + let mut ran = Vec::new(); + // The size it has in the index, so git has to read the file to tell whether it changed. + std::fs::write(root.join("a.txt"), "two\n").unwrap(); + let status = read_status("git", &root, false, false); + if marker.exists() { + ran.push("status"); + } + let _ = std::fs::remove_file(&marker); + std::fs::write(root.join("a.txt"), "one\ntwo\nthree\n").unwrap(); + let diff = read_diff("git", &root, None, false, &ide_core::CancellationToken::new()); + if marker.exists() { + ran.push("diff"); + } + // Control: the fixture's filter really runs when git is used directly. + let _ = std::fs::remove_file(&marker); + std::fs::write(root.join("a.txt"), "six\n").unwrap(); + let _ = std::process::Command::new("git").current_dir(&root).args(["status", "--porcelain"]).output(); + assert!(marker.exists(), "control: plain `git status` did not run the filter, fixture is not live"); + assert!(ran.is_empty(), "a read tool ran the repository's filter via: {ran:?}"); + assert_eq!(status.unwrap().len(), 1); + let diff = diff.unwrap(); + assert!(diff.contains("+three"), "{diff}"); + } + + #[test] + fn only_filters_of_the_repository_s_own_config_are_emptied() { + let listing = b"system\0filter.lfs.clean\ngit-lfs clean -- %f\0\ + global\0filter.lfs.process\ngit-lfs filter-process\0\ + global\0filter.lfs.required\ntrue\0\ + local\0core.bare\nfalse\0\ + local\0filter.a.clean\nx\0\ + worktree\0filter.b.process\ny\0\ + local\0filter.a.required\ntrue\0\ + command\0filter.c.clean\nz\0"; + assert_eq!( + repository_filter_overrides(listing).unwrap(), + ["-c", "filter.a.clean=", "-c", "filter.b.process=", "-c", "filter.a.required=false"] + ); + } + + fn git(dir: &Path, args: &[&str]) { + let out = std::process::Command::new("git").current_dir(dir).args(args).output().unwrap(); + assert!(out.status.success(), "git {args:?}: {}", String::from_utf8_lossy(&out.stderr)); + } + + /// A repository at `dir` with `a.txt` committed. + fn repository(dir: &Path, attributes: &[u8]) { + std::fs::create_dir_all(dir).unwrap(); + git(dir, &["init", "-q"]); + git(dir, &["config", "user.email", "t@example.test"]); + git(dir, &["config", "user.name", "t"]); + git(dir, &["config", "commit.gpgsign", "false"]); + std::fs::write(dir.join(".gitattributes"), attributes).unwrap(); + std::fs::write(dir.join("a.txt"), "one\n").unwrap(); + git(dir, &["add", "."]); + git(dir, &["commit", "-qm", "init"]); + } + + /// A shell command that leaves `marker` behind and passes its input on. + fn leaves(marker: &Path) -> String { + format!("echo x > '{}'; cat", marker.to_string_lossy().replace('\\', "/")) + } + + /// Whether a program of the fixture left `marker` behind; removes it. + fn left(marker: &Path) -> bool { + let was = marker.exists(); + let _ = std::fs::remove_file(marker); + was + } + + /// Reads the status and then the diff of `root` after rewriting `file`, then has plain git + /// read the status as a control. Each write keeps the size the file has in the index, so git + /// has to read it to tell whether it changed. + fn read_both(root: &Path, file: &Path, marker: &Path) -> (IdeResult>, IdeResult) { + let mut ran = Vec::new(); + std::fs::write(file, "two\n").unwrap(); + let status = read_status("git", root, false, false); + if left(marker) { + ran.push("status"); + } + std::fs::write(file, "six\n").unwrap(); + let diff = read_diff("git", root, None, false, &ide_core::CancellationToken::new()); + if left(marker) { + ran.push("diff"); + } + std::fs::write(file, "ten\n").unwrap(); + let _ = std::process::Command::new("git").current_dir(root).args(["status", "--porcelain"]).output(); + assert!(left(marker), "control: plain `git status` ran no program of the fixture, it is not live"); + assert!(ran.is_empty(), "a read tool ran a program of the repository via: {ran:?}"); + (status, diff) + } + + #[test] + fn status_and_diff_do_not_run_filters_of_any_repository_config() { + type Configure = fn(&Path, &str); + let cases: [(&str, Configure); 4] = [ + ("clean", |root, command| git(root, &["config", "filter.marker.clean", command])), + ("process", |root, command| git(root, &["config", "filter.marker.process", command])), + ("included file", |root, command| { + let included = format!("[filter \"marker\"]\n\tclean = \"{command}\"\n"); + std::fs::write(root.join("extra.gitconfig"), included).unwrap(); + git(root, &["config", "include.path", "../extra.gitconfig"]); + }), + ("worktree config", |root, command| { + git(root, &["config", "extensions.worktreeConfig", "true"]); + git(root, &["config", "--worktree", "filter.marker.clean", command]); + }), + ]; + for (case, configure) in cases { + let dir = tempfile::tempdir().unwrap(); + let root = dunce::canonicalize(dir.path()).unwrap().join("repo"); + let marker = root.with_file_name("marker"); + repository(&root, b"*.txt filter=marker\n"); + configure(&root, &leaves(&marker)); + let (status, diff) = read_both(&root, &root.join("a.txt"), &marker); + assert_eq!(status.unwrap().len(), 1, "{case}"); + let diff = diff.unwrap(); + assert!(diff.contains("+six"), "{case}: {diff}"); + } + } + + /// git splits `-c name=value` at the first `=` and takes no name that is not UTF-8 on every + /// platform, so such a filter cannot be emptied: the tools read nothing instead. + #[test] + fn status_and_diff_refuse_a_repository_filter_git_cannot_be_told_to_skip() { + for name in [&b"x=y"[..], &[0xE9]] { + let dir = tempfile::tempdir().unwrap(); + let root = dunce::canonicalize(dir.path()).unwrap().join("repo"); + let marker = root.with_file_name("marker"); + repository(&root, &[b"*.txt filter=", name, b"\n"].concat()); + let section = + [b"[filter \"", name, format!("\"]\n\tclean = \"{}\"\n", leaves(&marker)).as_bytes()].concat(); + let config = root.join(".git").join("config"); + std::fs::write(&config, [std::fs::read(&config).unwrap(), section].concat()).unwrap(); + let (status, diff) = read_both(&root, &root.join("a.txt"), &marker); + assert!(status.is_err() && diff.is_err(), "{name:?}"); + } + } + + /// git asks a submodule for its own status to tell whether it is dirty, under the + /// submodule's config. + #[test] + fn status_and_diff_do_not_run_filters_of_a_submodule() { + let dir = tempfile::tempdir().unwrap(); + let root = dunce::canonicalize(dir.path()).unwrap().join("repo"); + let (sub, marker) = (root.join("sub"), root.with_file_name("marker")); + repository(&root, b""); + repository(&sub, b"*.txt filter=marker\n"); + git(&root, &["add", "sub"]); + git(&root, &["commit", "-qm", "submodule"]); + git(&sub, &["config", "filter.marker.clean", &leaves(&marker)]); + git(&root, &["config", "diff.ignoreSubmodules", "none"]); + git(&root, &["config", "submodule.sub.ignore", "none"]); + let (status, diff) = read_both(&root, &sub.join("a.txt"), &marker); + status.unwrap(); + diff.unwrap(); + } + + /// `git diff` writes the index back when it finds a file whose stat changed and whose content + /// did not, and git runs the hooks a config names for `post-index-change` on that write. + #[test] + fn status_and_diff_do_not_run_hooks_of_the_repository_s_config() { + let dir = tempfile::tempdir().unwrap(); + let root = dunce::canonicalize(dir.path()).unwrap().join("repo"); + let marker = root.with_file_name("marker"); + repository(&root, b""); + git(&root, &["config", "hook.marker.event", "post-index-change"]); + let command = format!("echo x > '{}'", marker.to_string_lossy().replace('\\', "/")); + git(&root, &["config", "hook.marker.command", &command]); + let listed = std::process::Command::new("git") + .current_dir(&root) + .args(["hook", "list", "post-index-change"]) + .output() + .unwrap(); + if !String::from_utf8_lossy(&listed.stdout).contains("marker") { + return; // This git has no hooks named by a config. + } + // The content it has in the index, with a time of its own that is not the index's. + let touch = |seconds: u64| { + let file = std::fs::File::options().write(true).open(root.join("a.txt")).unwrap(); + file.set_modified(std::time::SystemTime::now() - std::time::Duration::from_secs(seconds)).unwrap(); + }; + let mut ran = Vec::new(); + touch(60); + let status = read_status("git", &root, false, false); + if left(&marker) { + ran.push("status"); + } + touch(120); + let diff = read_diff("git", &root, None, false, &ide_core::CancellationToken::new()); + if left(&marker) { + ran.push("diff"); + } + touch(180); + let _ = std::process::Command::new("git").current_dir(&root).args(["--no-pager", "diff"]).output(); + assert!(left(&marker), "control: plain `git diff` did not run the hook, fixture is not live"); + assert!(ran.is_empty(), "a read tool ran a hook of the repository's config via: {ran:?}"); + assert!(status.unwrap().is_empty()); + assert_eq!(diff.unwrap(), ""); + } + + /// `diff.submodule=diff` has git run `git diff` in the submodule, under the submodule's config. + #[test] + fn diff_does_not_run_a_diff_driver_of_a_submodule() { + let dir = tempfile::tempdir().unwrap(); + let root = dunce::canonicalize(dir.path()).unwrap().join("repo"); + let (sub, marker) = (root.join("sub"), root.with_file_name("marker")); + repository(&root, b""); + repository(&sub, b"*.txt diff=marker\n"); + git(&root, &["add", "sub"]); + git(&root, &["commit", "-qm", "submodule"]); + std::fs::write(sub.join("a.txt"), "one\ntwo\n").unwrap(); + git(&sub, &["commit", "-qam", "second"]); + git(&sub, &["config", "diff.marker.command", &leaves(&marker)]); + git(&root, &["config", "diff.submodule", "diff"]); + let diff = read_diff("git", &root, None, false, &ide_core::CancellationToken::new()); + let ran = left(&marker); + let _ = std::process::Command::new("git").current_dir(&root).args(["--no-pager", "diff"]).output(); + assert!(left(&marker), "control: plain `git diff` did not run the diff driver, fixture is not live"); + assert!(!ran, "git_diff ran the submodule's diff driver"); + let diff = diff.unwrap(); + assert!(diff.contains("Subproject commit"), "{diff}"); + assert_eq!(read_status("git", &root, false, false).unwrap().len(), 1); + } + #[cfg(unix)] #[test] fn paths_through_a_symlinked_root_compare_canonically() { diff --git a/crates/ide-host/src/ops/bytes.rs b/crates/ide-host/src/ops/bytes.rs index 8a2e6896e..b1e226990 100644 --- a/crates/ide-host/src/ops/bytes.rs +++ b/crates/ide-host/src/ops/bytes.rs @@ -17,9 +17,11 @@ pub async fn fs_read_range(_host: &Host, _window: &str, path: PathBuf, offset: u blocking(move || ide_vfs::read_range(&path, offset, len)).await } -/// An image file as a `data:` URL. -pub async fn fs_read_image(_host: &Host, _window: &str, path: PathBuf) -> IdeResult { - blocking(move || ide_vfs::read_data_url(&path)).await +/// An image file as a `data:` URL. A network path is read only from a share an open project is +/// on: the path may come from Markdown, and opening a share signs in to its server. +pub async fn fs_read_image(host: &Host, _window: &str, path: PathBuf) -> IdeResult { + let opened: Vec = host.projects().iter().map(|project| project.root.clone()).collect(); + blocking(move || ide_vfs::read_data_url(&path, &opened)).await } /// Commands of this module the remote server answers. diff --git a/crates/ide-host/src/ops/lsp.rs b/crates/ide-host/src/ops/lsp.rs index 9f6d936da..1fb266c23 100644 --- a/crates/ide-host/src/ops/lsp.rs +++ b/crates/ide-host/src/ops/lsp.rs @@ -884,7 +884,6 @@ fn plugin_file_edits( ) -> Result<(Vec, Option), String> { use ide_core::FileEdit; use ide_plugin::FileOperation; - let confine = !a.operations.is_empty() || a.open.is_some(); let confined = |path: &Path| ide_plugin::resolve_new_in_root(root, &path.to_string_lossy()); let writable = |path: &Path| ide_plugin::resolve_writable_in_root(root, &path.to_string_lossy()); let mut files = Vec::new(); @@ -913,13 +912,14 @@ fn plugin_file_edits( } } for (path, edits) in &a.edits { - let path = if confine { writable(path)? } else { path.clone() }; + let path = writable(path)?; if created_with_text.contains(&path) { return Err("put the text of a created file in its `create`, not in an edit".into()); } files.push(FileEdit::Edit { path: path.into(), edits: edits.clone(), version: None }); } - let open = a.open.as_deref().map(confined).transpose()?; + // Opening a file hands its text to the plugin, so a link is judged by the file it leads to. + let open = a.open.as_deref().map(writable).transpose()?; Ok((files, open)) } @@ -2341,6 +2341,138 @@ mod tests { assert_eq!((r.data.as_deref(), r.file), (Some("ref"), true)); } + /// Advisory #4b: an action carrying only text edits (no file operation, no `open`) must still + /// be confined to the project root and kept off git's own files. A plugin needs no grant to + /// offer a quick fix, so this is decided here. + #[test] + fn a_plugin_edits_only_action_stays_in_the_project_and_off_git() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("proj"); + std::fs::create_dir_all(root.join(".git")).unwrap(); + std::fs::write(root.join(".git/config"), "[core]\n").unwrap(); + std::fs::write(root.join("a.txt"), "a").unwrap(); + let root = dunce::canonicalize(&root).unwrap(); + let edits_only = |path: std::path::PathBuf| ide_plugin::CodeAction { + title: "Add import".into(), + kind: Some("quickfix".into()), + preferred: false, + edits: vec![(path, vec![TextEdit { range: Range::default(), new_text: "x".into() }])], + operations: Vec::new(), + open: None, + data: None, + file_action: false, + }; + // In the project root: allowed. + assert_eq!(plugin_code_action("acme.x", &root, edits_only(root.join("a.txt"))).disabled_reason, None); + // Outside the root: disabled (previously an edits-only action was not confined). + let outside = plugin_code_action("acme.x", &root, edits_only(tmp.path().join("evil.txt"))); + assert!( + outside.disabled_reason.as_deref().is_some_and(|r| r.contains("outside the project root")), + "{outside:?}" + ); + assert!(outside.edit.unwrap().files.is_empty()); + // Into .git: disabled. + let git = plugin_code_action("acme.x", &root, edits_only(root.join(".git/config"))); + assert!(git.disabled_reason.as_deref().is_some_and(|r| r.contains("git's own files")), "{git:?}"); + } + + /// Advisory #4b: a quick fix's file operations and its `open` are held to the same rule as its + /// edits, under every name that reaches git's own files. + #[test] + fn a_plugin_action_cannot_change_or_open_git_files() { + use ide_plugin::FileOperation::{Create, Delete, Rename}; + let tmp = tempfile::tempdir().unwrap(); + for dir in ["proj/.git/hooks", "worktree"] { + std::fs::create_dir_all(tmp.path().join(dir)).unwrap(); + } + let root = dunce::canonicalize(tmp.path().join("proj")).unwrap(); + let worktree = dunce::canonicalize(tmp.path().join("worktree")).unwrap(); + std::fs::write( + root.join(".git/config"), + "[core] +", + ) + .unwrap(); + std::fs::write(root.join("a.txt"), "a").unwrap(); + std::fs::write( + worktree.join(".git"), + "gitdir: elsewhere +", + ) + .unwrap(); + std::fs::write(tmp.path().join("profile.txt"), "outside").unwrap(); + let action = + |edits: Vec, operations: Vec| ide_plugin::CodeAction { + title: "Add import".into(), + kind: Some("quickfix".into()), + preferred: false, + edits: edits + .into_iter() + .map(|path| (path, vec![TextEdit { range: Range::default(), new_text: "x".into() }])) + .collect(), + operations, + open: None, + data: None, + file_action: false, + }; + let refused = |root: &Path, action: ide_plugin::CodeAction, why: &str| { + let offered = plugin_code_action("acme.x", root, action); + assert!(offered.disabled_reason.as_deref().is_some_and(|r| r.contains(why)), "{offered:?}"); + assert!(offered.edit.unwrap().files.is_empty()); + }; + + for git in [root.join(".git/config"), root.join(".GIT/config"), ".git/hooks/pre-commit".into()] { + refused(&root, action(vec![git], Vec::new()), "git's own files"); + } + for operation in [ + Create { + path: root.join(".git/hooks/pre-commit"), + text: "#!/bin/sh +" + .into(), + overwrite: false, + }, + Rename { from: root.join("a.txt"), to: root.join(".git/config") }, + Rename { from: root.join(".git/config"), to: root.join("b.txt") }, + Delete { path: root.join(".git/config") }, + ] { + refused(&root, action(Vec::new(), vec![operation]), "git's own files"); + } + // Opening a file hands its text to the plugin's providers. + let opens = + |path: std::path::PathBuf| ide_plugin::CodeAction { open: Some(path), ..action(Vec::new(), Vec::new()) }; + refused(&root, opens(root.join(".git/config")), "git's own files"); + // An edit lands in the file a link leads to, and `open` shows that file: both are judged by it. + #[cfg(unix)] + { + std::os::unix::fs::symlink(tmp.path().join("profile.txt"), root.join("link.txt")).unwrap(); + std::os::unix::fs::symlink(root.join(".git/config"), root.join("alias")).unwrap(); + refused(&root, action(vec![root.join("link.txt")], Vec::new()), "outside the project root"); + refused(&root, action(vec![root.join("alias")], Vec::new()), "git's own files"); + refused(&root, opens(root.join("link.txt")), "outside the project root"); + refused(&root, opens(root.join("alias")), "git's own files"); + } + // A linked worktree's `.git` file, under the short name Windows opens it by as well. + if cfg!(windows) && worktree.join("GIT~1").exists() { + for operation in [ + Delete { path: worktree.join("GIT~1") }, + Create { + path: worktree.join("GIT~1"), + text: "gitdir: other +" + .into(), + overwrite: true, + }, + ] { + refused(&worktree, action(Vec::new(), vec![operation]), "git's own files"); + } + refused(&worktree, action(vec![worktree.join("GIT~1")], Vec::new()), "git's own files"); + refused(&worktree, opens(worktree.join("GIT~1")), "git's own files"); + } else { + eprintln!("short name not tried: this volume keeps none"); + } + } + #[test] fn every_completion_kind_the_plugin_api_documents_is_understood() { let wit = include_str!("../../../../wit/providers.wit"); diff --git a/crates/ide-host/src/plugins/api.rs b/crates/ide-host/src/plugins/api.rs index 366f130f6..b4bc0eac9 100644 --- a/crates/ide-host/src/plugins/api.rs +++ b/crates/ide-host/src/plugins/api.rs @@ -1263,6 +1263,8 @@ pub(super) fn commit_draft( let Some(r) = ide_vcs::Repo::discover(&repo.root.0).map_err(failed)? else { return Err(HostError::NotFound(format!("{} is not in a git repository", repo.root.0.display()))); }; + // Both callers have checked that the window's project is trusted. + r.set_execution_allowed(true); let pending = r.pending_changes(&repo.options, budget, &|path| Ok(workspace.vcs_encoding(path))).map_err( |e| match e.kind { ide_core::error::ErrorKind::InvalidInput => HostError::Invalid(e.message), @@ -2093,7 +2095,7 @@ impl HostApi for PluginApi { } fn vcs_status(&self, root: &Path) -> Result, HostError> { - let Some(repo) = ide_vcs::Repo::discover(root).map_err(failed)? else { return Ok(None) }; + let Some(repo) = self.repo(root)? else { return Ok(None) }; let info = repo.branch_info().map_err(failed)?; let changed = repo.status().map_err(failed)?.len(); Ok(Some(RepositoryStatus { @@ -2119,7 +2121,7 @@ impl HostApi for PluginApi { if !workspace.is_some_and(|w| w.trusted(root)) { return Err(HostError::Failed("the project is in Restricted Mode: git does not run there".into())); } - let Some(repo) = ide_vcs::Repo::discover(root).map_err(failed)? else { + let Some(repo) = self.repo(root)? else { return Err(HostError::NotFound("the project is not in a git repository".into())); }; match repo.fetch(cancel, &|_| {}) { @@ -3936,6 +3938,16 @@ fn valid_range(document: &ide_text::Document, range: ide_core::Range) -> bool { } impl PluginApi { + /// The repository of `root`, which may execute only when `root` is a trusted project. + /// Status needs it there: a restricted repository is read without its config, so it knows + /// no upstream. The other reads use [`Self::read_repo`] and never execute. + fn repo(&self, root: &Path) -> Result, HostError> { + let repo = ide_vcs::Repo::discover(root).map_err(failed)?; + if let Some(repo) = &repo { + repo.set_execution_allowed(self.shared.workspace().is_some_and(|w| w.trusted(root))); + } + Ok(repo) + } fn read_repo(&self, root: &Path) -> Result, HostError> { if !self.shared.workspace().is_some_and(|w| w.trusted(root)) { return Err(HostError::Failed("the project is in Restricted Mode: git does not run there".into())); @@ -4200,6 +4212,36 @@ mod workbench_tests { std::fs::write(&path, "x".repeat(5 << 20)).unwrap(); assert!(api.vcs_diff(&root, None, None, &cancel).is_err()); } + /// Security audit lead 2: status has no trust check to stop it, so in Restricted Mode the + /// repository it opens must not run what its config names. + #[test] + fn vcs_status_runs_no_repository_program_in_restricted_mode() { + let dir = tempfile::tempdir().unwrap(); + let root = dunce::canonicalize(dir.path()).unwrap(); + git(&root, &["init", "-q"]); + git(&root, &["config", "user.name", "Test"]); + git(&root, &["config", "user.email", "test@example.com"]); + git(&root, &["config", "commit.gpgsign", "false"]); + std::fs::write(root.join("a.txt"), "base\n").unwrap(); + git(&root, &["add", "."]); + git(&root, &["commit", "-qm", "base"]); + let marker = dir.path().join("FILTER-RAN"); + let clean = format!("echo x > '{}'; cat", marker.to_str().unwrap().replace('\\', "/")); + git(&root, &["config", "filter.marker.clean", &clean]); + std::fs::write(root.join(".gitattributes"), "*.txt filter=marker\n").unwrap(); + // Same length as the committed text: only reading the file tells them apart. + std::fs::write(root.join("a.txt"), "BASE\n").unwrap(); + + let restricted = api(&root, false, None); + let status = restricted.vcs_status(&root); + assert!(!marker.exists(), "status ran the repository's clean filter in Restricted Mode"); + assert_eq!(status.unwrap().unwrap().changed_files, 2); + + // Control: the same call runs the fixture's filter in a trusted project. + api(&root, true, None).vcs_status(&root).unwrap(); + assert!(marker.exists(), "control: a trusted status did not run the clean filter, fixture is not live"); + } + #[cfg(unix)] #[test] fn vcs_reads_never_run_repository_configured_helpers() { diff --git a/crates/ide-host/src/plugins/market.rs b/crates/ide-host/src/plugins/market.rs index 7a1a82780..d3d93dabc 100644 --- a/crates/ide-host/src/plugins/market.rs +++ b/crates/ide-host/src/plugins/market.rs @@ -14,6 +14,7 @@ use ide_extension::{MANIFEST_FILE, Manifest}; use semver::Version; use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; +use std::time::Duration; /// The official marketplace: the index the release workflow publishes with /// the app, next to the signed packages of the official plugins. @@ -93,22 +94,29 @@ fn safe_suggestions(paths: Vec) -> Vec { paths.into_iter().filter(ok).take(MAX_SUGGESTIONS).collect() } +/// Whether `url` is read over the network: `http(s)://…`, not `file://…` or a local path. +fn remote(url: &str) -> bool { + url.starts_with("https://") || url.starts_with("http://") +} + +/// Whether an index read from `base` may send the IDE to `href` (resolved). One read over +/// the network names nothing on this machine: the IDE would read the file for it, or sign +/// in to the share it names. +pub fn reachable_from(base: &str, href: &str) -> bool { + !remote(base) || remote(href) +} + +/// The slowest transfer a read over the network waits for, in bytes per second: with +/// `READ_GRACE`, the largest index is given up on after about five minutes, the largest +/// package after a little over an hour. +const SLOWEST_READ: usize = 64 << 10; +/// What a read over the network may take before its first byte. +const READ_GRACE: Duration = Duration::from_secs(30); + /// Reads `url`: `http(s)://…`, `file://…` or a local path. pub fn read_url(url: &str, max: usize) -> Result, String> { - if url.starts_with("https://") || url.starts_with("http://") { - let client = ide_http::client::shared().map_err(|e| e.message)?; - let url = url.to_owned(); - return super::api::net_runtime().block_on(async move { - let response = client.get(&url).send().await.map_err(|e| format!("{url}: {e}"))?; - if !response.status().is_success() { - return Err(format!("{url}: HTTP {}", response.status())); - } - let bytes = response.bytes().await.map_err(|e| format!("{url}: {e}"))?; - if bytes.len() > max { - return Err(format!("{url}: larger than {} MiB", max >> 20)); - } - Ok(bytes.to_vec()) - }); + if remote(url) { + return read_remote(url, max, READ_GRACE + Duration::from_secs((max / SLOWEST_READ) as u64)); } let path = url.strip_prefix("file://").unwrap_or(url); let bytes = std::fs::read(path).map_err(|e| format!("{path}: {e}"))?; @@ -118,6 +126,35 @@ pub fn read_url(url: &str, max: usize) -> Result, String> { Ok(bytes) } +/// Reads `url` over http(s): no more than `max` bytes, whatever the server announces or +/// sends, and within `deadline` from the request to the last byte. The marketplace is read +/// with nobody asking, so its server decides neither how much is kept nor for how long. +fn read_remote(url: &str, max: usize, deadline: Duration) -> Result, String> { + let client = ide_http::client::shared().map_err(|e| e.message)?; + super::api::net_runtime().block_on(async move { + let failed = |e: reqwest::Error| match e.is_timeout() { + true => format!("{url}: no answer in time"), + false => format!("{url}: {e}"), + }; + let too_large = || format!("{url}: larger than {} MiB", max >> 20); + let mut response = client.get(url).timeout(deadline).send().await.map_err(failed)?; + if !response.status().is_success() { + return Err(format!("{url}: HTTP {}", response.status())); + } + if response.content_length().is_some_and(|announced| announced > max as u64) { + return Err(too_large()); + } + let mut bytes = Vec::new(); + while let Some(chunk) = response.chunk().await.map_err(failed)? { + if bytes.len() + chunk.len() > max { + return Err(too_large()); + } + bytes.extend_from_slice(&chunk); + } + Ok(bytes) + }) +} + /// The index at `url`, following `moved`, with downloads made absolute against /// where it was read from. fn read_index(url: &str) -> IdeResult { @@ -132,12 +169,25 @@ fn read_index(url: &str) -> IdeResult { if url.starts_with("https://") && !next.starts_with("https://") { return Err(IdeError::invalid(format!("the marketplace moved to {next}, which isn't https"))); } + if !reachable_from(&url, &next) { + return Err(IdeError::invalid("the marketplace moved to an address that is not on the network")); + } url = next; } None => { for e in &mut index.plugins { e.download = resolve(&url, &e.download); } + // Judged here, against where this index was read: the address in use may be + // an index on this machine that moved to it. + let listed = index.plugins.len(); + index.plugins.retain(|e| reachable_from(&url, &e.download)); + if index.plugins.len() < listed { + tracing::warn!( + "plugin marketplace: {} downloads are not on the network; they are not offered", + listed - index.plugins.len() + ); + } return Ok(index); } } @@ -295,13 +345,23 @@ impl PluginHost { entry.name, entry.version, entry.engine ))); } - let bytes = read_url(&resolve(&url, &entry.download), MAX_PACKAGE).map_err(IdeError::unavailable)?; + let download = resolve(&url, &entry.download); + // An index read by this version holds only downloads it may reach. The one kept by an + // earlier version does not say where it was read: it is judged against the marketplace in use. + if !reachable_from(&url, &download) { + return Err(IdeError::invalid(format!( + "the marketplace's download of {} {} is not on the network", + entry.id, entry.version + ))); + } + let bytes = read_url(&download, MAX_PACKAGE).map_err(IdeError::unavailable)?; let downloads = self.plugins_dir.join(DOWNLOADS); std::fs::create_dir_all(&downloads)?; let archive = downloads.join(format!("{}-{}.{}", entry.id, entry.version, package::PACKAGE_EXTENSION)); std::fs::write(&archive, bytes)?; - let dir = self.unpack_package(&archive)?; + let dir = self.unpack_package(&archive); let _ = std::fs::remove_file(&archive); + let dir = dir?; let manifest = Manifest::load(&dir).map_err(IdeError::invalid)?; let files = self.package_files_in(&dir)?; let digest = package::package_digest(&dir, &files)?; @@ -508,4 +568,43 @@ mod tests { assert_eq!(safe_suggestions(paths.iter().map(|p| p.to_string()).collect()), ["pom.xml", "config/routes.rb"]); assert_eq!(safe_suggestions(vec!["a".into(); 20]).len(), MAX_SUGGESTIONS); } + + #[test] + fn an_index_read_over_the_network_reaches_only_the_network() { + let base = "https://x.dev/market/index.json"; + let reaches = |href: &str| reachable_from(base, &resolve(base, href)); + for href in ["p/a.oiplugin", "../a.oiplugin", "https://cdn.dev/a"] { + assert!(reaches(href), "{href}"); + } + for href in ["/etc/passwd", "//attacker/share/a", "file:///etc/passwd", "file://attacker/share/a"] { + assert!(!reaches(href), "{href}"); + } + #[cfg(windows)] + for href in [r"C:\Users\me\.ssh\id_ed25519", r"\\attacker\share\a", r"\\?\UNC\attacker\share\a"] { + assert!(!reaches(href), "{href}"); + } + // A local index names what whoever put it there chose. + assert!(reachable_from("/srv/market/index.json", "/srv/market/a.oiplugin")); + } + + #[test] + fn a_server_that_stops_answering_does_not_hold_the_read() { + use std::io::{BufRead, BufReader, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://127.0.0.1:{}/index.json", listener.local_addr().unwrap().port()); + std::thread::spawn(move || { + for mut stream in listener.incoming().flatten() { + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut line = String::new(); + while reader.read_line(&mut line).unwrap_or(0) > 0 && line != "\r\n" { + line.clear(); + } + // The headers, then nothing for longer than the read waits. + let _ = stream.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\n"); + std::thread::sleep(Duration::from_secs(5)); + } + }); + let err = read_remote(&url, MAX_INDEX, Duration::from_millis(300)).unwrap_err(); + assert!(err.contains("no answer in time"), "{err}"); + } } diff --git a/crates/ide-host/src/plugins/mod.rs b/crates/ide-host/src/plugins/mod.rs index e0cfbd654..b599cbce6 100644 --- a/crates/ide-host/src/plugins/mod.rs +++ b/crates/ide-host/src/plugins/mod.rs @@ -63,7 +63,7 @@ pub use icons::{ pub(crate) use inlay_hints::merge as merge_inlay_hints; pub use market::{ INDEX_FILE as MARKET_INDEX_FILE, MarketPlugin, OFFICIAL_INDEX as OFFICIAL_MARKET_INDEX, OFFICIAL_PUBLISHER, - read_url as read_market_url, resolve as resolve_market_href, + reachable_from as market_href_reachable_from, read_url as read_market_url, resolve as resolve_market_href, }; pub use points::{COMMANDS, CommandDecl, register_builtin_points}; pub use providers::{ProviderKind, set_editor_providers}; diff --git a/crates/ide-host/tests/distribution.rs b/crates/ide-host/tests/distribution.rs index ee1335602..7950efc47 100644 --- a/crates/ide-host/tests/distribution.rs +++ b/crates/ide-host/tests/distribution.rs @@ -209,6 +209,49 @@ fn write_index( path.to_string_lossy().into_owned() } +/// Security audit lead 5: an index entry's id and version name the download file, +/// so an entry whose id or version would place it outside the downloads folder +/// is not offered, and nothing is written for it. +#[test] +fn market_entry_ids_cannot_place_the_download_outside_the_downloads_folder() { + let f = fixture(); + let market = f.work.join("market"); + std::fs::create_dir_all(&market).unwrap(); + let key = PublisherKey::from_seed([3; 32]); + let (_, hello) = signed_hello(&market, "1.0.0", &key, &f.host); + // Never read: an entry that is not offered is not downloaded. + std::fs::write(market.join("junk.bin"), b"attacker bytes").unwrap(); + // The id leaves the folder on every platform (`.downloads` exists by then); the version + // only where `..` is resolved before the path is opened (Windows). + for (id, version) in [("../../escape", "1.0.0"), ("acme.hello", "1.0.0/../../../escape")] { + let entry = IndexEntry { id: id.into(), version: version.into(), download: "junk.bin".into(), ..hello.clone() }; + let url = write_index(&market, &key, vec![entry], vec![], false); + f.host.set_market_url(Some(url)); + f.host.refresh_market().unwrap(); + + assert!(f.host.market_plugins().is_empty(), "{id} {version} is offered"); + let err = f.host.download_market_plugin(id).unwrap_err(); + assert!(err.message.contains("is not in the marketplace"), "{id} {version}: {}", err.message); + } + let downloads = f.plugins_dir.join(".downloads"); + let mut escaped = Vec::new(); + let mut pending = vec![f._tmp.path().to_path_buf()]; + while let Some(dir) = pending.pop() { + for e in std::fs::read_dir(&dir).unwrap().flatten() { + let path = e.path(); + if path.is_dir() { + pending.push(path); + } else if path.extension().is_some_and(|x| x == "oiplugin") + && !path.starts_with(&downloads) + && !path.starts_with(&market) + { + escaped.push(path); + } + } + } + assert!(escaped.is_empty(), "the download was written outside {}: {escaped:?}", downloads.display()); +} + /// First run: nothing cached yet, so listing the marketplace reads its index /// instead of showing an empty one; later listings use the index read. #[test] @@ -366,6 +409,181 @@ fn a_moved_marketplace_is_read_where_it_moved_to() { assert!(err.message.contains("moved more than"), "{}", err.message); } +/// A loopback HTTP server whose answer to every request `respond` writes; the URL of its index. +fn serve_with(respond: impl Fn(&mut std::net::TcpStream) + Send + 'static) -> String { + use std::io::{BufRead, BufReader}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://127.0.0.1:{}/index.json", listener.local_addr().unwrap().port()); + std::thread::spawn(move || { + for mut stream in listener.incoming().flatten() { + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut line = String::new(); + while reader.read_line(&mut line).unwrap_or(0) > 0 && line != "\r\n" { + line.clear(); + } + respond(&mut stream); + } + }); + url +} + +/// A loopback HTTP server answering every request with `body`; the URL of its index. +fn serve(body: String) -> String { + use std::io::Write; + serve_with(move |stream| { + let _ = write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + }) +} + +/// An index is read up to the size an index may have, whatever its server says or sends: +/// the marketplace is read again every day, with nobody asking. +#[test] +fn an_index_larger_than_an_index_may_be_is_not_read_to_its_end() { + use std::io::Write; + use std::sync::atomic::{AtomicUsize, Ordering}; + let f = fixture(); + // Announced: refused on its headers, before any of it is read. + f.host.set_market_url(Some(serve_with(|stream| { + let _ = stream.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 1073741824\r\nConnection: close\r\n\r\n{"); + }))); + let err = f.host.refresh_market().unwrap_err(); + assert!(err.message.contains("larger than 16 MiB"), "{}", err.message); + + // Not announced: the read stops at the limit, and the server is left with the rest. + const BODY: usize = 96 << 20; + let sent = Arc::new(AtomicUsize::new(0)); + f.host.set_market_url(Some(serve_with({ + let sent = sent.clone(); + move |stream| { + let _ = stream.write_all(b"HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n"); + let chunk = [b' '; 64 << 10]; + let mut written = 0; + while written < BODY && stream.write_all(&chunk).is_ok() { + written += chunk.len(); + } + sent.store(written.max(1), Ordering::SeqCst); + } + }))); + let err = f.host.refresh_market().unwrap_err(); + assert!(err.message.contains("larger than 16 MiB"), "{}", err.message); + let waiting = std::time::Instant::now(); + while sent.load(Ordering::SeqCst) == 0 && waiting.elapsed() < std::time::Duration::from_secs(30) { + std::thread::sleep(std::time::Duration::from_millis(20)); + } + let sent = sent.load(Ordering::SeqCst); + assert!(sent > 0 && sent < BODY, "the server sent {sent} of {BODY} bytes"); +} + +/// An index read over the network names downloads on the network: not a file of this +/// machine, nor a share the machine would sign in to. +#[test] +fn an_index_read_over_the_network_cannot_name_a_local_download() { + let f = fixture(); + let market = f.work.join("market"); + std::fs::create_dir_all(&market).unwrap(); + let key = PublisherKey::from_seed([3; 32]); + let (package, entry) = signed_hello(&market, "1.0.0", &key, &f.host); + let path = package.to_string_lossy().into_owned(); + // A package that installs, so only where the index was read from can refuse it. + for download in [path.clone(), format!("file://{path}")] { + let entry = IndexEntry { download: download.clone(), ..entry.clone() }; + let local = write_index(&market, &key, vec![entry], vec![], false); + f.host.set_market_url(Some(local.clone())); + f.host.refresh_market().unwrap(); + let dir = f.host.download_market_plugin("acme.hello").unwrap(); + assert_eq!(f.host.preview_install(&dir).unwrap().version, "1.0.0", "{download}"); + + f.host.set_market_url(Some(serve(std::fs::read_to_string(&local).unwrap()))); + assert!(f.host.refresh_market().unwrap().is_empty(), "{download} is offered"); + let err = f.host.download_market_plugin("acme.hello").unwrap_err(); + assert!(err.message.contains("is not in the marketplace"), "{download}: {}", err.message); + } +} + +/// Nor when an index on this machine is what sent the IDE to the one on the network. +#[test] +fn an_index_reached_through_a_local_one_cannot_name_a_local_download() { + let f = fixture(); + let market = f.work.join("market"); + std::fs::create_dir_all(&market).unwrap(); + let key = PublisherKey::from_seed([3; 32]); + let (package, entry) = signed_hello(&market, "1.0.0", &key, &f.host); + let entry = IndexEntry { download: package.to_string_lossy().into_owned(), ..entry }; + let served = serve(std::fs::read_to_string(write_index(&market, &key, vec![entry], vec![], false)).unwrap()); + let old = f.work.join("old"); + std::fs::create_dir_all(&old).unwrap(); + let moved = serde_json::json!({ "format": package::INDEX_FORMAT, "moved": served }).to_string(); + std::fs::write(old.join("index.json"), moved).unwrap(); + f.host.set_market_url(Some(old.join("index.json").to_string_lossy().into_owned())); + + assert!(f.host.refresh_market().unwrap().is_empty(), "the local download is offered"); + let err = f.host.download_market_plugin("acme.hello").unwrap_err(); + assert!(err.message.contains("is not in the marketplace"), "{}", err.message); +} + +/// The index kept from the last read does not say where it was read from: what an earlier +/// version kept is judged against the marketplace in use. +#[test] +fn a_kept_index_cannot_name_a_local_download_for_a_marketplace_on_the_network() { + let f = fixture(); + let market = f.work.join("market"); + std::fs::create_dir_all(&market).unwrap(); + let key = PublisherKey::from_seed([3; 32]); + let (package, entry) = signed_hello(&market, "1.0.0", &key, &f.host); + let entry = IndexEntry { download: package.to_string_lossy().into_owned(), ..entry }; + let kept = write_index(&market, &key, vec![entry], vec![], false); + std::fs::copy(kept, f.plugins_dir.join("market-index.json")).unwrap(); + let again = PluginHost::new(f.plugins_dir.clone(), Arc::new(|_: Option<&str>, _| {}), Limits::default()).unwrap(); + // Never read: the check comes before any request. + again.set_market_url(Some("https://market.invalid/index.json".into())); + + assert_eq!(again.market_plugins()[0].entry.id, "acme.hello"); + let err = again.download_market_plugin("acme.hello").unwrap_err(); + assert!(err.message.contains("is not on the network"), "{}", err.message); +} + +/// Nor may it send the IDE to an index on this machine. +#[test] +fn an_index_read_over_the_network_cannot_move_to_a_local_one() { + let f = fixture(); + let market = f.work.join("market"); + std::fs::create_dir_all(&market).unwrap(); + let key = PublisherKey::from_seed([3; 32]); + let (_, entry) = signed_hello(&market, "1.0.0", &key, &f.host); + let local = write_index(&market, &key, vec![entry], vec![], false); + let moved = serde_json::json!({ "format": package::INDEX_FORMAT, "moved": local }).to_string(); + f.host.set_market_url(Some(serve(moved))); + + let err = f.host.refresh_market().unwrap_err(); + assert!(err.message.contains("is not on the network"), "{}", err.message); + assert!(f.host.market_plugins().is_empty(), "the local index was read"); +} + +/// A download that turns out not to be a package is not kept. +#[test] +fn a_download_that_is_not_a_package_leaves_nothing_behind() { + let f = fixture(); + let market = f.work.join("market"); + std::fs::create_dir_all(&market).unwrap(); + let key = PublisherKey::from_seed([3; 32]); + let (_, entry) = signed_hello(&market, "1.0.0", &key, &f.host); + std::fs::write(market.join("junk.bin"), b"not a package").unwrap(); + let entry = IndexEntry { download: "junk.bin".into(), ..entry }; + f.host.set_market_url(Some(write_index(&market, &key, vec![entry], vec![], false))); + f.host.refresh_market().unwrap(); + + let err = f.host.download_market_plugin("acme.hello").unwrap_err(); + assert!(err.message.contains("not a plugin package"), "{}", err.message); + let left: Vec<_> = std::fs::read_dir(f.plugins_dir.join(".downloads")) + .map(|dir| dir.flatten().map(|e| e.file_name()).collect()) + .unwrap_or_default(); + assert!(left.is_empty(), "{left:?}"); +} + /// `dir/index.json` listing `publishers` and `plugins`; its path. fn index_of(dir: &Path, publishers: Vec, plugins: Vec) -> String { let index = Index { format: package::INDEX_FORMAT, publishers, plugins, ..Index::default() }; diff --git a/crates/ide-host/tests/network_images.rs b/crates/ide-host/tests/network_images.rs new file mode 100644 index 000000000..e18e0ede3 --- /dev/null +++ b/crates/ide-host/tests/network_images.rs @@ -0,0 +1,70 @@ +//! An image path may come from Markdown someone else wrote, and on Windows opening a network +//! share signs in to its server with the user's credentials: the host reads an image from a +//! share only when a project is open on it. The share here is this machine's own `C$` over +//! loopback. +#![cfg(windows)] + +use ide_core::NullSink; +use ide_host::ops::bytes::fs_read_image; +use ide_host::{Host, HostConfig}; +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +/// `path` as seen through the admin share of `server`, a name of this machine. +fn over_smb(server: &str, path: &Path) -> PathBuf { + let s = path.to_str().unwrap(); + PathBuf::from(format!(r"\\{server}\{}${}", &s[..1], &s[2..])) +} + +#[test] +fn images_on_a_share_are_read_only_where_a_project_is_open() { + let tmp = tempfile::tempdir().unwrap(); + let local = dunce::canonicalize(tmp.path()).unwrap(); + let root = local.join("project"); + std::fs::create_dir_all(root.join("docs")).unwrap(); + for image in [root.join("docs/a.png"), local.join("outside.png")] { + std::fs::write(image, b"\x89PNG dummy").unwrap(); + } + let runtime = tokio::runtime::Builder::new_multi_thread().worker_threads(2).enable_all().build().unwrap(); + let _guard = runtime.enter(); + let host = Host::new(HostConfig { + config_dir: tmp.path().join("config"), + data_dir: tmp.path().join("data"), + local_data_dir: tmp.path().join("data"), + languages: Arc::new(ide_language::LanguageRegistry::load(None).unwrap()), + syntax: Arc::default(), + sink_for: Box::new(|_| Arc::new(NullSink)), + owner: |w| w, + runtime: runtime.handle().clone(), + project_environment: false, + }); + let read = |path: PathBuf| runtime.block_on(fs_read_image(&host, "main", path)); + + let in_project = over_smb("127.0.0.1", &root.join("docs/a.png")); + let beside_project = over_smb("127.0.0.1", &local.join("outside.png")); + let other_server = over_smb("localhost", &root.join("docs/a.png")); + for path in [&in_project, &beside_project, &other_server] { + assert!(std::fs::read(path).is_ok(), "control: {} is not reachable over SMB", path.display()); + } + let wanted = read(root.join("docs/a.png")).unwrap(); + + // No project is open on any share. + for path in [&in_project, &beside_project, &other_server] { + assert!(read(path.clone()).is_err(), "read {} with no project on its share", path.display()); + } + + let project_root = over_smb("127.0.0.1", &root); + host.trust.decide(&project_root, true, false).unwrap(); + host.open_project("main", &project_root).unwrap(); + assert_eq!(read(in_project.clone()).unwrap(), wanted); + // The whole share is one sign-in: a file outside the project folder is on it too. + assert_eq!(read(beside_project.clone()).unwrap(), wanted); + assert_eq!(read(PathBuf::from(in_project.to_str().unwrap().replace('\\', "/"))).unwrap(), wanted); + // The path as the webview holds it: the project root it was given, joined with `/`. + let root_in_webview = ide_core::ipc_path::to_ipc(&host.project("main").unwrap().root); + assert_eq!(read(PathBuf::from(format!("{root_in_webview}/docs/a.png"))).unwrap(), wanted); + assert!(read(other_server.clone()).is_err(), "read {} from a server no project is on", other_server.display()); + + host.close_project("main"); + assert!(read(in_project.clone()).is_err(), "read {} after its project closed", in_project.display()); +} diff --git a/crates/ide-host/tests/plugins.rs b/crates/ide-host/tests/plugins.rs index 9128da081..e9a562629 100644 --- a/crates/ide-host/tests/plugins.rs +++ b/crates/ide-host/tests/plugins.rs @@ -6574,3 +6574,73 @@ fn document_indexer_covers_workspace_members() { .unwrap(); assert!(f.plugins.navigation_at(&f.root, &outside, 1, ide_core::Pos::new(0, 4), Some("main"), |_| None).is_none()); } + +/// Advisory #4: `project.write` (Medium) must not reach VCS metadata such as `.git/hooks` or +/// `.git/config`, which git runs as code; the `.git` is inside the project root, so root +/// confinement alone does not stop it. +#[test] +fn project_write_cannot_reach_git_metadata() { + let f = fixture(Limits::default()); + let init = std::process::Command::new("git").current_dir(&f.project).args(["init", "-q"]).output().unwrap(); + assert!(init.status.success(), "git init"); + f.install(&misbehave_with(f._tmp.path(), "misbehave-project-writer", "project.write")); + // The grant works for an ordinary path, so each refusal below is the `.git` rule and nothing else. + assert_eq!(f.run("misbehave", "projwrite", Some("src/fresh.txt")).unwrap(), None); + for rel in [".git/hooks/pre-commit", ".git/config", ".GIT/config", "src/../.git/config"] { + let before = std::fs::read(f.project.join(rel)).ok(); + let err = f.run("misbehave", "projwrite", Some(rel)).unwrap_err(); + assert!(err.contains("git's own files"), "{rel}: {err}"); + assert_eq!(std::fs::read(f.project.join(rel)).ok(), before, "{rel}"); + } +} + +/// In a linked worktree or a submodule `.git` is a file naming the real git folder. Replacing it, +/// under any name that opens it, would point git at a folder of the plugin's choosing or leave the +/// checkout without a repository. +#[test] +fn project_write_cannot_replace_the_git_file_of_a_linked_worktree() { + let f = fixture(Limits::default()); + let pointer = f.project.join(".git"); + let gitdir = "gitdir: /elsewhere/.git/worktrees/project\n"; + std::fs::write(&pointer, gitdir).unwrap(); + f.install(&misbehave_with(f._tmp.path(), "misbehave-project-writer", "project.write")); + assert_eq!(f.run("misbehave", "projwrite", Some("src/fresh.txt")).unwrap(), None); + let mut spellings = vec![".git", ".GIT", ".git.", ".git ", ".git::$DATA"]; + // Windows opens a file by its short name too, on a volume that keeps them. + if cfg!(windows) && f.project.join("GIT~1").exists() { + spellings.extend(["GIT~1", "git~1", "GIT~1.", "GIT~1 ", "GIT~1::$DATA"]); + } else { + eprintln!("short name not tried: this volume keeps none"); + } + for rel in spellings { + let err = f.run("misbehave", "projwrite", Some(rel)).unwrap_err(); + assert!(err.contains("git's own files"), "{rel}: {err}"); + assert_eq!(std::fs::read_to_string(&pointer).unwrap(), gitdir, "{rel}"); + } +} + +/// A link already in the project is written through, so it is judged by where it points. +#[cfg(unix)] +#[test] +fn project_write_judges_a_link_by_its_target() { + let f = fixture(Limits::default()); + let config = f.project.join(".git").join("config"); + std::fs::create_dir(f.project.join(".git")).unwrap(); + std::fs::write(&config, "[core]\n").unwrap(); + let secret = f._tmp.path().join("secret.txt"); + std::os::unix::fs::symlink(&config, f.project.join("alias")).unwrap(); + std::os::unix::fs::symlink(&secret, f.project.join("escape")).unwrap(); + f.install(&misbehave_with(f._tmp.path(), "misbehave-project-writer", "project.write")); + + // A folder that is not there, in front of the link, does not keep it from being followed. + for rel in ["alias", "missing/../alias"] { + let err = f.run("misbehave", "projwrite", Some(rel)).unwrap_err(); + assert!(err.contains("git's own files"), "{rel}: {err}"); + assert_eq!(std::fs::read_to_string(&config).unwrap(), "[core]\n", "{rel}"); + } + for rel in ["escape", "missing/../escape"] { + let err = f.run("misbehave", "projwrite", Some(rel)).unwrap_err(); + assert!(err.contains("outside the project root"), "{rel}: {err}"); + assert_eq!(std::fs::read_to_string(&secret).unwrap(), "top secret", "{rel}"); + } +} diff --git a/crates/ide-host/tests/trust_vcs_sweep.rs b/crates/ide-host/tests/trust_vcs_sweep.rs new file mode 100644 index 000000000..c568c4160 --- /dev/null +++ b/crates/ide-host/tests/trust_vcs_sweep.rs @@ -0,0 +1,308 @@ +//! Security audit lead 2: every `vcs_*` command Restricted Mode allows, run against a +//! repository whose config names a program for a signed commit, an index read, an external +//! diff, a clean filter and a missing object. None may run. +use ide_core::{NullSink, RequestId}; +use ide_host::{Host, HostConfig, ops, ops::vcs}; +use std::collections::BTreeSet; +use std::future::Future; +use std::path::{Path, PathBuf}; +use std::pin::Pin; +use std::process::{Command, Stdio}; +use std::sync::Arc; + +/// Allowed in Restricted Mode and not swept: none of them starts git for the project. +const NOT_THE_PROJECT_REPOSITORY: [&str; 7] = [ + "vcs_clone", // clones a URL the user gives into a new folder + "vcs_diff_hunks", // compares two texts + "vcs_detected_git", // looks for the git executable + "vcs_shelves", // the shelf and the changelists are files of the IDE + "vcs_shelf_patch", + "vcs_changelists_load", + "vcs_write_patch", // writes a patch the UI already holds +]; + +fn git(root: &Path, args: &[&str]) -> String { + let output = Command::new("git").current_dir(root).args(args).output().unwrap(); + assert!(output.status.success(), "{args:?}: {}", String::from_utf8_lossy(&output.stderr)); + String::from_utf8(output.stdout).unwrap().trim().to_owned() +} + +fn spelled(path: &Path) -> String { + path.to_str().unwrap().replace('\\', "/") +} + +struct Armed { + root: PathBuf, + first: String, + head: String, + /// A commit whose tree is missing: git asks the promisor for it. + promised: String, +} + +/// A repository that starts a program of its own for a signed commit (`gpg`), an index read +/// (`fsmonitor`), a file diff (`extdiff`), a touched file (`filter`) and a missing object +/// (`promisor`). Each program leaves a file named after itself in `markers`. Other ways in +/// (textconv, process filters, merge drivers) are not armed here. +fn arm(root: &Path, markers: &Path) -> Armed { + std::fs::create_dir_all(root.join("src")).unwrap(); + git(root, &["init", "-q", "-b", "main"]); + for (key, value) in [("user.name", "Test"), ("user.email", "test@example.com"), ("commit.gpgsign", "false")] { + git(root, &["config", key, value]); + } + std::fs::write(root.join("src/a.txt"), "one\ntwo\n").unwrap(); + git(root, &["add", "."]); + git(root, &["commit", "-qm", "first"]); + let first = git(root, &["rev-parse", "HEAD"]); + std::fs::write(root.join("src/a.txt"), "one\ntwo\nthree\n").unwrap(); + git(root, &["commit", "-qam", "second"]); + + // HEAD again, carrying a signature header: `git log` then starts the verifier. + let body = git(root, &["cat-file", "commit", "HEAD"]); + let (headers, message) = body.split_once("\n\n").unwrap(); + let signed = format!( + "{headers}\ngpgsig -----BEGIN PGP SIGNATURE-----\n \n iQEzBAABCAAdFiEEfake\n -----END PGP SIGNATURE-----\n\n{message}\n" + ); + let mut child = Command::new("git") + .current_dir(root) + .args(["hash-object", "-t", "commit", "-w", "--stdin"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .unwrap(); + std::io::Write::write_all(&mut child.stdin.take().unwrap(), signed.as_bytes()).unwrap(); + let head = String::from_utf8(child.wait_with_output().unwrap().stdout).unwrap().trim().to_owned(); + git(root, &["update-ref", "HEAD", &head]); + + // A branch whose tip has a tree of its own, which is then dropped from the object database. + git(root, &["checkout", "-q", "-b", "promised", &first]); + std::fs::write(root.join("only-here.txt"), "promised\n").unwrap(); + git(root, &["add", "."]); + git(root, &["commit", "-qm", "promised"]); + let promised = git(root, &["rev-parse", "HEAD"]); + let tree = git(root, &["rev-parse", "HEAD^{tree}"]); + git(root, &["checkout", "-q", "main"]); + std::fs::remove_file(root.join(".git/objects").join(&tree[..2]).join(&tree[2..])).unwrap(); + + let marker = |name: &str| spelled(&markers.join(name)); + let script = |name: &str| { + let path = root.join(".git").join(format!("{name}.sh")); + std::fs::write(&path, format!("#!/bin/sh\necho x > '{}'\nexit 1\n", marker(name))).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + spelled(&path) + }; + let settings = [ + ("log.showSignature", "true".to_owned()), + ("gpg.program", script("gpg")), + ("core.fsmonitor", script("fsmonitor")), + ("diff.external", script("extdiff")), + ("filter.marker.clean", format!("echo x > '{}'; cat", marker("filter"))), + ("extensions.partialClone", "origin".to_owned()), + ("remote.origin.url", spelled(root)), + ("remote.origin.promisor", "true".to_owned()), + ("remote.origin.partialclonefilter", "blob:none".to_owned()), + ("remote.origin.uploadpack", format!("echo x > '{}'; git-upload-pack", marker("promisor"))), + ]; + std::fs::write(root.join(".gitattributes"), "*.txt filter=marker\n").unwrap(); + // Same length as the committed text: only reading the file tells them apart. + std::fs::write(root.join("src/a.txt"), "ONE\ntwo\nthree\n").unwrap(); + for (key, value) in &settings { + git(root, &["config", key, value]); + } + Armed { root: root.to_path_buf(), first, head, promised } +} + +fn ran(markers: &Path) -> Vec { + let mut names: Vec = std::fs::read_dir(markers) + .unwrap() + .flatten() + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + names +} + +fn clear(markers: &Path) { + let _ = std::fs::remove_dir_all(markers); + std::fs::create_dir_all(markers).unwrap(); +} + +type Call<'a> = (&'static str, &'static str, Pin + 'a>>); + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn restricted_vcs_commands_run_no_program_of_the_repository() { + let temp = tempfile::tempdir().unwrap(); + let base = dunce::canonicalize(temp.path()).unwrap(); + let markers = base.join("markers"); + clear(&markers); + let project = arm(&base.join("project"), &markers); + let external = arm(&base.join("external"), &markers); + let host = Host::new(HostConfig { + config_dir: base.join("config"), + data_dir: base.join("data"), + local_data_dir: base.join("data"), + languages: Arc::new(ide_language::LanguageRegistry::load(None).unwrap()), + syntax: Arc::default(), + sink_for: Box::new(|_| Arc::new(NullSink)), + owner: |w| w, + runtime: tokio::runtime::Handle::current(), + project_environment: false, + }); + host.trust.decide(&project.root, false, false).unwrap(); + clear(&markers); + assert!(!host.open_project("main", &project.root).unwrap().is_trusted()); + let host = &host; + let w = "main"; + let file = project.root.join("src/a.txt"); + let root = || Some(project.root.clone()); + let log = || Some(external.root.clone()); + + macro_rules! call { + ($command:literal, $what:literal, $call:expr) => { + ($command, $what, Box::pin(async { $call.await.is_ok() }) as Pin + '_>>) + }; + } + macro_rules! sync { + ($command:literal, $what:literal, $call:expr) => { + ($command, $what, Box::pin(async { $call.is_ok() }) as Pin + '_>>) + }; + } + let calls: Vec = vec![ + call!("vcs_status", "", ops::vcs::status(host, w)), + sync!("vcs_roots", "", vcs::vcs_roots(host, w)), + call!("vcs_branch_info", "", vcs::vcs_branch_info(host, w, root())), + call!("vcs_line_hunks", "", vcs::vcs_line_hunks(host, w, file.clone())), + call!("vcs_file_at", "", vcs::vcs_file_at(host, w, file.clone(), None)), + call!("vcs_blame", "", vcs::vcs_blame(host, w, file.clone(), None, None)), + call!("vcs_block_history", "", vcs::vcs_block_history(host, w, RequestId(1), file.clone(), None, (0, 1))), + call!("vcs_log", "", vcs::vcs_log(host, w, root(), Default::default())), + call!("vcs_log_refs", "", vcs::vcs_log_refs(host, w, root())), + call!("vcs_log_users", "", vcs::vcs_log_users(host, w, root())), + call!("vcs_log_details", "", vcs::vcs_log_details(host, w, root(), project.head.clone())), + call!( + "vcs_log_details_batch", + "", + vcs::vcs_log_details_batch( + host, + w, + root(), + vec![project.head.clone(), project.first.clone()], + RequestId(2) + ) + ), + call!( + "vcs_revision_children", + "", + vcs::vcs_revision_children(host, w, root(), project.head.clone(), "src".into()) + ), + call!( + "vcs_revision_children", + "missing tree", + vcs::vcs_revision_children(host, w, root(), project.promised.clone(), String::new()) + ), + call!("vcs_revision_files", "", vcs::vcs_revision_files(host, w, root(), project.head.clone())), + call!("vcs_revision_files", "missing tree", vcs::vcs_revision_files(host, w, root(), project.promised.clone())), + call!("vcs_commit_files", "", vcs::vcs_commit_files(host, w, root(), project.head.clone())), + call!("vcs_commits_changes", "", vcs::vcs_commits_changes(host, w, root(), vec![project.head.clone()])), + call!("vcs_branches", "", vcs::vcs_branches(host, w, root())), + call!("vcs_stashes", "", vcs::vcs_stashes(host, w, root())), + call!("vcs_conflict_versions", "", vcs::vcs_conflict_versions(host, w, file.clone())), + call!("vcs_diff_text", "", vcs::vcs_diff_text(host, w, file.clone(), false)), + call!("vcs_config_get", "", vcs::vcs_config_get(host, w, root(), "user.name".into())), + call!("vcs_tags", "", vcs::vcs_tags(host, w, root())), + call!("vcs_remotes", "", vcs::vcs_remotes(host, w, root())), + call!("vcs_worktrees", "", vcs::vcs_worktrees(host, w, root())), + call!("vcs_diff_with_worktree", "", vcs::vcs_diff_with_worktree(host, w, root(), "HEAD".into())), + call!("vcs_compare_scope", "", vcs::vcs_compare_scope(host, w, vec![project.root.clone()])), + call!( + "vcs_compare_paths", + "", + vcs::vcs_compare_paths(host, w, vec![project.root.clone()], "HEAD".into(), None, RequestId(3)) + ), + call!( + "vcs_diff_revisions", + "", + vcs::vcs_diff_revisions(host, w, root(), project.first.clone(), project.head.clone()) + ), + call!( + "vcs_diff_numstat", + "", + vcs::vcs_diff_numstat(host, w, root(), project.first.clone(), project.head.clone(), Vec::new()) + ), + call!("vcs_merge_base", "", vcs::vcs_merge_base(host, w, root(), project.first.clone(), project.head.clone())), + call!("vcs_has_hook", "", vcs::vcs_has_hook(host, w, root(), "pre-push".into())), + // A worktree outside the project, opened for its history only. + call!("vcs_open_external_log", "", vcs::vcs_open_external_log(host, w, external.root.clone())), + call!("vcs_log", "external", vcs::vcs_log(host, w, log(), Default::default())), + call!("vcs_log_users", "external", vcs::vcs_log_users(host, w, log())), + call!("vcs_log_details", "external", vcs::vcs_log_details(host, w, log(), external.head.clone())), + call!( + "vcs_revision_children", + "external, missing tree", + vcs::vcs_revision_children(host, w, log(), external.promised.clone(), String::new()) + ), + call!("vcs_revision_files", "external", vcs::vcs_revision_files(host, w, log(), external.head.clone())), + call!("vcs_commit_files", "external", vcs::vcs_commit_files(host, w, log(), external.head.clone())), + // Last: it rewrites nothing of an existing repository, but it does touch it. + call!("vcs_init", "", vcs::vcs_init(host, w, project.root.clone())), + ]; + + // The sweep covers what `trust.rs` allows: a newly allowed `vcs_*` command fails here until + // it is swept or listed as not touching the project's repository. + let source = include_str!("../src/trust.rs"); + let start = source.find("pub fn restricted_command_allowed").unwrap(); + let end = source.find("pub fn projectless_command_allowed").unwrap(); + let allowed: BTreeSet<&str> = source[start..end].split('"').filter(|s| s.starts_with("vcs_")).collect(); + let swept: BTreeSet<&str> = calls.iter().map(|(command, _, _)| *command).collect(); + let known: BTreeSet<&str> = swept.iter().copied().chain(NOT_THE_PROJECT_REPOSITORY).collect(); + assert!(allowed.len() > 30, "trust.rs was not read: {allowed:?}"); + assert_eq!(allowed, known, "the sweep and Restricted Mode's allowed vcs commands differ"); + for command in &known { + assert!(ide_host::trust::restricted_command_allowed(command), "{command} is not allowed in Restricted Mode"); + } + + let mut executed = Vec::new(); + let mut failed = Vec::new(); + let opening = ran(&markers); + if !opening.is_empty() { + executed.push(format!("open_project: {}", opening.join(", "))); + } + for (command, what, call) in calls { + clear(&markers); + let ok = call.await; + let name = if what.is_empty() { command.to_owned() } else { format!("{command} ({what})") }; + let programs = ran(&markers); + if !programs.is_empty() { + executed.push(format!("{name}: {}", programs.join(", "))); + } + if !ok { + failed.push(name); + } + } + + // Control: plain git, in the same repository, starts every one of the fixture's programs. + clear(&markers); + for args in [&["log", "-1", "--format=%H"][..], &["status", "--porcelain"], &["ls-tree", &project.promised]] { + let _ = Command::new("git").current_dir(&project.root).args(args).output().unwrap(); + } + let _ = Command::new("git").current_dir(&project.root).args(["diff"]).output().unwrap(); + assert_eq!( + ran(&markers), + ["extdiff", "filter", "fsmonitor", "gpg", "promisor"], + "control: the fixture is not live" + ); + + assert!(executed.is_empty(), "Restricted Mode ran a program of the repository:\n{}", executed.join("\n")); + // Only the listings of the commit whose tree is gone have nothing to return. + assert_eq!( + failed, + [ + "vcs_revision_children (missing tree)", + "vcs_revision_files (missing tree)", + "vcs_revision_children (external, missing tree)" + ] + ); +} diff --git a/crates/ide-plugin/src/sandbox.rs b/crates/ide-plugin/src/sandbox.rs index cffa3bbe1..378580ac1 100644 --- a/crates/ide-plugin/src/sandbox.rs +++ b/crates/ide-plugin/src/sandbox.rs @@ -118,6 +118,24 @@ pub fn inside_roots(roots: &[PathBuf], path: &str) -> Result { /// be created) or that names a symlink itself (to delete or rename it): the /// directory part is resolved through symlinks, the final name is kept as is, /// and the result must stay inside `root`. +/// Whether a path (relative to the project root) goes through git's own `.git` folder or file. +/// git runs what it finds in `.git` (hooks, `gpg.program`, filters), so a content write must +/// never reach it. Names are normalized for trailing dots and spaces and NTFS stream suffixes; +/// the caller also checks the short-name-resolved path via [`resolve_real`]. +pub fn names_git_dir(rel: &Path) -> bool { + rel.components().any(|part| { + let name = part.as_os_str().to_string_lossy(); + let name = name.split(':').next().unwrap_or_default().trim_end_matches(['.', ' ']); + name.eq_ignore_ascii_case(".git") + }) +} + +/// A resolved write destination that reaches git's own `.git` metadata, judged by the name +/// given and by the name the file system resolves it to. +fn reaches_git_dir(root: &Path, real: &Path) -> bool { + real.strip_prefix(root).is_ok_and(names_git_dir) || resolve_real(real).strip_prefix(root).is_ok_and(names_git_dir) +} + pub fn resolve_new_in_root(root: &Path, path: &str) -> Result { let outside = || format!("{path}: access outside the project root is not allowed"); if path.is_empty() { @@ -154,6 +172,9 @@ pub fn resolve_new_in_root(root: &Path, path: &str) -> Result { if !real.starts_with(&root) { return Err(outside()); } + if reaches_git_dir(&root, &real) { + return Err(format!("{path}: writing git's own files is not allowed")); + } Ok(real) } @@ -165,6 +186,9 @@ pub fn resolve_writable_in_root(root: &Path, path: &str) -> Result IdeResu run_paths_env(root, args, paths, None, &[]) } +const RESTRICTED_CONFIG: [&str; 8] = [ + "core.fsmonitor=false", + "core.hooksPath=", + // `git diff` writes the index back after refreshing it, and that write runs the hooks a + // config names for `post-index-change`, which `core.hooksPath` does not reach. + "diff.autoRefreshIndex=false", + "hook.post-index-change.enabled=false", + "diff.external=", + "core.attributesFile=", + "submodule.recurse=false", + "log.showSignature=false", +]; + +/// The keys that name a signature verifier; `gpg.openpgp.program` is an alias of `gpg.program`. +const VERIFIER_KEYS: [&str; 4] = ["gpg.program", "gpg.openpgp.program", "gpg.ssh.program", "gpg.x509.program"]; + +/// A program path git opens directly and cannot find, as nothing exists below a regular file. +/// An empty name is not enough: Git for Windows looks it up on `PATH` and runs a file named `.exe`. +fn no_program() -> &'static std::ffi::OsStr { + static PATH: OnceLock = OnceLock::new(); + PATH.get_or_init(|| match std::env::current_exe() { + Ok(exe) if exe.is_absolute() => exe.join("no-program").into_os_string(), + _ => Default::default(), + }) +} + +/// Keeps `cmd`, a git that reads a repository, from running what that repository's config names +/// for a read: the fsmonitor hook, hooks, the external diff, a signature verifier and a lazy +/// fetch. Call it before the subcommand is added. Clean and process filters have a key per +/// driver ([`filter_overrides_from`] empties those); signing, transport and credential helpers +/// are not covered. +pub fn restrict(cmd: &mut Command) { + for setting in RESTRICTED_CONFIG { + cmd.arg("-c").arg(setting); + } + for key in VERIFIER_KEYS { + let mut setting = std::ffi::OsString::from(format!("{key}=")); + setting.push(no_program()); + cmd.arg("-c").arg(setting); + } + cmd.env_remove("GIT_EXTERNAL_DIFF").env("GIT_NO_LAZY_FETCH", "1"); +} + /// Builds a `git` command with the environment every invocation needs. pub(crate) fn command(root: &Path) -> IdeResult { let restricted = !crate::execution_allowed(); @@ -215,16 +258,7 @@ pub(crate) fn command(root: &Path) -> IdeResult { }; let mut cmd = Command::new(binary); if restricted { - for setting in [ - "core.fsmonitor=false", - "core.hooksPath=", - "diff.external=", - "core.attributesFile=", - "submodule.recurse=false", - ] { - cmd.arg("-c").arg(setting); - } - cmd.env_remove("GIT_EXTERNAL_DIFF").env("GIT_NO_LAZY_FETCH", "1"); + restrict(&mut cmd); } cmd.current_dir(root) .arg("-c") @@ -385,6 +419,13 @@ fn run_cancellable_raw_stdin( run_capture(root, args, stdin, cancel, usize::MAX) } +/// Options for a `git diff` that reads the working tree in Restricted Mode: git asks a submodule +/// with changes for its own status, and for its own diff when `diff.submodule` says so, and both +/// run under the submodule's config. +pub(crate) fn submodule_guards() -> &'static [&'static str] { + if crate::execution_allowed() { &[] } else { &["--ignore-submodules=dirty", "--submodule=short"] } +} + /// Neutralizes repository clean/process filters for a read of working-tree bytes. /// Git still applies them with --no-ext-diff and --no-textconv. pub(crate) fn filter_overrides(root: &Path, cancel: &CancellationToken) -> IdeResult> { @@ -392,15 +433,27 @@ pub(crate) fn filter_overrides(root: &Path, cancel: &CancellationToken) -> IdeRe return Ok(Vec::new()); } let config = run_bounded(root, &["config", "--null", "--list"], cancel, 4 << 20)?; + filter_overrides_from(&config.stdout) +} + +/// `-c` arguments that empty every clean and process filter in `config`, the output of +/// `git config --null --list`. Fails when one of them cannot be emptied this way: git splits +/// `-c name=value` at the first `=`, and a name that is not UTF-8 does not reach git as it is +/// on every platform. +pub fn filter_overrides_from(config: &[u8]) -> IdeResult> { let mut overrides = Vec::new(); - for record in config.stdout_str().split('\0') { - let key = record.split('\n').next().unwrap_or_default(); - if key.starts_with("filter.") - && (key.ends_with(".clean") || key.ends_with(".process") || key.ends_with(".required")) - { - overrides.push("-c".into()); - overrides.push(format!("{key}={}", if key.ends_with(".required") { "false" } else { "" })); + for record in config.split(|b| *b == 0) { + let key = record.split(|b| *b == b'\n').next().unwrap_or_default(); + let required = key.ends_with(b".required"); + if !key.starts_with(b"filter.") || !(key.ends_with(b".clean") || key.ends_with(b".process") || required) { + continue; } + let Some(key) = std::str::from_utf8(key).ok().filter(|key| !key.contains('=')) else { + let key = String::from_utf8_lossy(key); + return Err(IdeError::invalid(format!("git cannot be told to skip the filter setting `{key}`"))); + }; + overrides.push("-c".into()); + overrides.push(format!("{key}={}", if required { "false" } else { "" })); } Ok(overrides) } @@ -760,6 +813,25 @@ mod command_session_tests { } } +#[cfg(test)] +mod restrict_tests { + use super::*; + + #[test] + fn restrict_names_a_verifier_git_opens_directly_and_cannot_find() { + let mut cmd = Command::new("git"); + restrict(&mut cmd); + let args: Vec<_> = cmd.get_args().map(|arg| arg.to_string_lossy().into_owned()).collect(); + let programs: Vec<_> = args.iter().filter_map(|arg| arg.split_once(".program=")).collect(); + assert_eq!(programs.len(), 4, "{args:?}"); + for (key, program) in programs { + let program = Path::new(program); + assert!(program.is_absolute(), "{key}.program is {program:?}: git would look that name up on PATH"); + assert!(!program.exists(), "{key}.program is {program:?}, which exists"); + } + } +} + #[cfg(test)] mod batching_tests { use super::{WINDOWS_LIMIT, base_command_line_len, batch_paths, command_line_limit}; @@ -836,12 +908,10 @@ mod index_lock_tests { run(dir.path(), &["init", "-q", "-b", "main"], None).unwrap(); std::fs::write(dir.path().join("file"), "main\n").unwrap(); run(dir.path(), &["add", "file"], None).unwrap(); - run( - dir.path(), - &["-c", "user.name=Test", "-c", "user.email=test@example.com", "commit", "-qm", "initial"], - None, - ) - .unwrap(); + // git started outside an execution scope is restricted and has no signing program, so the + // user's `commit.gpgsign` must not reach this commit. + let identity = ["-c", "user.name=Test", "-c", "user.email=test@example.com", "-c", "commit.gpgsign=false"]; + run(dir.path(), &[&identity[..], &["commit", "-qm", "initial"]].concat(), None).unwrap(); run(dir.path(), &["branch", "other"], None).unwrap(); dir } diff --git a/crates/ide-vcs/src/compare.rs b/crates/ide-vcs/src/compare.rs index 6e1bc738b..9af90cbaf 100644 --- a/crates/ide-vcs/src/compare.rs +++ b/crates/ide-vcs/src/compare.rs @@ -151,18 +151,12 @@ impl Repos { } let resolved = resolved.stdout_str().trim().to_owned(); // Disable textconv/external diff; these are also safe in untrusted projects. - let names = - run(&["diff", "--name-status", "-z", "-M", "--no-ext-diff", "--no-textconv", &resolved, "--"])?; - let stats = run(&[ - "diff", - "--numstat", - "-z", - "--no-renames", - "--no-ext-diff", - "--no-textconv", - &resolved, - "--", - ])?; + let diff = |options: &[&str]| { + let fixed = &["diff", "--no-ext-diff", "--no-textconv"][..]; + run(&[fixed, cli::submodule_guards(), options, &[resolved.as_str(), "--"]].concat()) + }; + let names = diff(&["--name-status", "-z", "-M"])?; + let stats = diff(&["--numstat", "-z", "--no-renames"])?; let stats: std::collections::HashMap<_, _> = ops::parse_numstat(&stats.stdout_str()) .into_iter() .map(|(path, added, removed)| (path, (added, removed))) @@ -207,8 +201,7 @@ impl Repos { }); } if files.iter().any(|f| f.kind == ChangeKind::Renamed) { - let renamed = - run(&["diff", "--numstat", "-z", "-M", "--no-ext-diff", "--no-textconv", &resolved, "--"])?; + let renamed = diff(&["--numstat", "-z", "-M"])?; let indexes: std::collections::HashMap<_, _> = files .iter() .enumerate() @@ -289,7 +282,13 @@ mod tests { use std::process::Command; fn git(root: &Path, args: &[&str]) { - let output = Command::new("git").current_dir(root).args(args).output().unwrap(); + let output = Command::new("git") + .current_dir(root) + .args(args) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_GLOBAL", if cfg!(windows) { "NUL" } else { "/dev/null" }) + .output() + .unwrap(); assert!(output.status.success(), "{args:?}: {}", String::from_utf8_lossy(&output.stderr)); } @@ -506,4 +505,429 @@ mod tests { git(&root, &["ls-files"]); assert!(marker.exists(), "control invocation did not run fsmonitor"); } + + /// git splits `-c name=value` at the first `=` and takes no name that is not UTF-8 on every + /// platform, so such a filter cannot be emptied: Restricted Mode compares nothing instead. + #[test] + fn restricted_comparison_refuses_a_filter_git_cannot_be_told_to_skip() { + for name in [&b"x=y"[..], &[0xE9]] { + let temp = tempfile::tempdir().unwrap(); + let root = normalized(temp.path()).join("repo"); + let marker = root.with_file_name("marker"); + init(&root, "main"); + std::fs::write(root.join(".gitattributes"), [b"*.txt filter=", name, b"\n"].concat()).unwrap(); + let command = format!("echo x > '{}'; cat", marker.to_str().unwrap().replace('\\', "/")); + let section = [b"[filter \"", name, format!("\"]\n\tclean = \"{command}\"\n").as_bytes()].concat(); + let config = root.join(".git/config"); + std::fs::write(&config, [std::fs::read(&config).unwrap(), section].concat()).unwrap(); + // The size it has in the index, so git has to read the file to tell whether it changed. + write(&root, "src/move.txt", "more\n"); + let repos = Repos::discover(&root, &|_| false).unwrap(); + for repo in repos.all() { + repo.set_execution_allowed(false); + } + let result = repos.compare_paths(std::slice::from_ref(&root), "main", None, &CancellationToken::new()); + let ran = marker.exists(); + let _ = std::fs::remove_file(&marker); + write(&root, "src/move.txt", "less\n"); + let _ = Command::new("git").current_dir(&root).args(["status", "--porcelain"]).output(); + assert!(marker.exists(), "control: plain `git status` did not run the filter, fixture is not live"); + assert!(!ran, "Restricted Mode ran the repository's filter {name:?}"); + assert!(result.is_ok_and(|r| r.repositories[0].error.is_some()), "{name:?}"); + } + } + + /// A repository with a second one in `sub`, which has `attributes`, committed as a submodule; + /// returns the two roots and the path of the marker a program of the fixture leaves behind. + fn with_submodule(temp: &Path, attributes: &str) -> (PathBuf, PathBuf, PathBuf) { + let root = normalized(temp).join("repo"); + let sub = root.join("sub"); + init(&root, "main"); + init(&sub, "main"); + write(&sub, ".gitattributes", attributes); + git(&sub, &["add", "."]); + git(&sub, &["commit", "-qm", "attributes"]); + git(&root, &["add", "sub"]); + git(&root, &["commit", "-qm", "submodule"]); + (root.clone(), sub, root.with_file_name("marker")) + } + + /// git asks a submodule with changes for its own status, which runs under the submodule's config. + #[test] + fn restricted_reads_do_not_run_filters_of_a_submodule() { + let temp = tempfile::tempdir().unwrap(); + let (root, sub, marker) = with_submodule(temp.path(), "*.txt filter=marker\n"); + let command = format!("echo x > '{}'; cat", marker.to_str().unwrap().replace('\\', "/")); + git(&sub, &["config", "filter.marker.clean", &command]); + let repos = Repos::discover(&root, &|_| false).unwrap(); + for repo in repos.all() { + repo.set_execution_allowed(false); + } + let mut ran = Vec::new(); + // The size it has in the index, so git has to read the file to tell whether it changed. + write(&sub, "src/move.txt", "more\n"); + let compared = repos.compare_paths(std::slice::from_ref(&root), "main", None, &CancellationToken::new()); + if marker.exists() { + ran.push("compare_paths"); + } + let _ = std::fs::remove_file(&marker); + write(&sub, "src/move.txt", "less\n"); + let patch = repos.at_root(&root).unwrap().working_tree_patch(None, None, 1 << 20, &CancellationToken::new()); + if marker.exists() { + ran.push("working_tree_patch"); + } + let _ = std::fs::remove_file(&marker); + write(&sub, "src/move.txt", "same\n"); + let _ = Command::new("git").current_dir(&root).args(["status", "--porcelain"]).output(); + assert!(marker.exists(), "control: plain `git status` did not run the filter, fixture is not live"); + assert!(ran.is_empty(), "Restricted Mode ran the submodule's filter via: {ran:?}"); + assert!(compared.unwrap().repositories.iter().all(|r| r.error.is_none())); + patch.unwrap(); + } + + /// git runs the hooks a config names for `post-index-change` when a command writes the index + /// back, as `git diff` does after refreshing it. + #[test] + fn restricted_reads_do_not_run_hooks_of_the_repository_s_config() { + let temp = tempfile::tempdir().unwrap(); + let root = normalized(temp.path()).join("repo"); + let marker = root.with_file_name("marker"); + init(&root, "main"); + git(&root, &["config", "hook.marker.event", "post-index-change"]); + let command = format!("echo x > '{}'", marker.to_str().unwrap().replace('\\', "/")); + git(&root, &["config", "hook.marker.command", &command]); + let listed = Command::new("git").current_dir(&root).args(["hook", "list", "post-index-change"]).output(); + if !String::from_utf8_lossy(&listed.unwrap().stdout).contains("marker") { + return; // This git has no hooks named by a config. + } + // The content it has in the index, with a time of its own that is not the index's. + let touch = |seconds: u64| { + let file = std::fs::File::options().write(true).open(root.join("src/move.txt")).unwrap(); + file.set_modified(std::time::SystemTime::now() - std::time::Duration::from_secs(seconds)).unwrap(); + }; + let repos = Repos::discover(&root, &|_| false).unwrap(); + let repo = repos.at_root(&root).unwrap(); + repo.set_execution_allowed(false); + let mut ran = Vec::new(); + touch(60); + let compared = repos.compare_paths(std::slice::from_ref(&root), "main", None, &CancellationToken::new()); + if marker.exists() { + ran.push("compare_paths"); + } + let _ = std::fs::remove_file(&marker); + touch(120); + let patch = repo.working_tree_patch(None, None, 1 << 20, &CancellationToken::new()); + if marker.exists() { + ran.push("working_tree_patch"); + } + let _ = std::fs::remove_file(&marker); + touch(180); + let _ = Command::new("git").current_dir(&root).args(["--no-pager", "diff"]).output(); + assert!(marker.exists(), "control: plain `git diff` did not run the hook, fixture is not live"); + assert!(ran.is_empty(), "Restricted Mode ran a hook of the repository's config via: {ran:?}"); + assert!(compared.unwrap().repositories[0].error.is_none()); + assert_eq!(patch.unwrap(), ""); + } + + /// `diff.submodule=diff` has git run `git diff` in the submodule, under the submodule's config. + #[test] + fn restricted_patch_does_not_run_a_diff_driver_of_a_submodule() { + let temp = tempfile::tempdir().unwrap(); + let (root, sub, marker) = with_submodule(temp.path(), "*.txt diff=marker\n"); + write(&sub, "src/move.txt", "move\nagain\n"); + git(&sub, &["commit", "-qam", "second"]); + let command = format!("echo x > '{}'; cat", marker.to_str().unwrap().replace('\\', "/")); + git(&sub, &["config", "diff.marker.command", &command]); + git(&root, &["config", "diff.submodule", "diff"]); + let repos = Repos::discover(&root, &|_| false).unwrap(); + for repo in repos.all() { + repo.set_execution_allowed(false); + } + let patch = repos.at_root(&root).unwrap().working_tree_patch(None, None, 1 << 20, &CancellationToken::new()); + let ran = marker.exists(); + let _ = std::fs::remove_file(&marker); + let _ = Command::new("git").current_dir(&root).args(["--no-pager", "diff", "HEAD"]).output(); + assert!(marker.exists(), "control: plain `git diff` did not run the diff driver, fixture is not live"); + assert!(!ran, "Restricted Mode ran the submodule's diff driver"); + let patch = patch.unwrap(); + assert!(patch.contains("Subproject commit"), "{patch}"); + } + + /// Replaces HEAD with a copy of itself carrying a (fake) gpgsig header. `armor` labels the + /// signature and so selects git's verifier: `PGP SIGNATURE`, `SSH SIGNATURE` or + /// `SIGNED MESSAGE` (X.509). + fn sign_head(root: &Path, armor: &str) { + use std::io::Write; + let out = Command::new("git").current_dir(root).args(["cat-file", "commit", "HEAD"]).output().unwrap(); + let body = String::from_utf8(out.stdout).unwrap(); + let (headers, message) = body.split_once("\n\n").unwrap(); + // Drop the signature of an earlier call so only the new one remains. + let mut kept = Vec::new(); + let mut in_sig = false; + for line in headers.lines() { + if line.starts_with("gpgsig") { + in_sig = true; + } else if !(in_sig && line.starts_with(' ')) { + in_sig = false; + kept.push(line); + } + } + let headers = kept.join("\n"); + let signed = format!( + "{headers}\ngpgsig -----BEGIN {armor}-----\n \n iQEzBAABCAAdFiEEfake\n -----END {armor}-----\n\n{message}" + ); + let mut child = Command::new("git") + .current_dir(root) + .args(["hash-object", "-t", "commit", "-w", "--stdin"]) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .spawn() + .unwrap(); + child.stdin.take().unwrap().write_all(signed.as_bytes()).unwrap(); + let id = String::from_utf8(child.wait_with_output().unwrap().stdout).unwrap(); + git(root, &["update-ref", "HEAD", id.trim()]); + } + + /// Writes a script into `.git` that records its own execution. Returns the marker file it + /// creates and its own path, spelled the way git config takes it. + fn verifier_script(root: &Path) -> (PathBuf, String) { + let marker = root.join("executed-marker"); + let script = root.join(".git/verifier.sh"); + std::fs::write( + &script, + format!("#!/bin/sh\necho x > '{}'\nexit 1\n", marker.to_str().unwrap().replace('\\', "/")), + ) + .unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + (marker, script.to_str().unwrap().replace('\\', "/")) + } + + /// Security audit lead 2: Restricted Mode `git log` must not run the repository's signature verifier. + #[test] + fn restricted_log_does_not_execute_signature_verifier() { + let temp = tempfile::tempdir().unwrap(); + let root = normalized(temp.path()); + init(&root, "main"); + write(&root, "src/move.txt", "move\nsigned change\n"); + git(&root, &["commit", "-qam", "signed"]); + sign_head(&root, "PGP SIGNATURE"); + let (marker, script) = verifier_script(&root); + git(&root, &["config", "log.showSignature", "true"]); + git(&root, &["config", "gpg.program", &script]); + + let repos = Repos::discover(&root, &|_| false).unwrap(); + let repo = repos.all().first().unwrap(); + repo.set_execution_allowed(false); + let file = root.join("src/move.txt"); + type Call<'a> = (&'static str, Box); + let calls: [Call; 4] = [ + ("log_users", Box::new(|| assert_eq!(repo.log_users().unwrap(), ["Test "]))), + // Restricted Mode does not ask git to verify here: it reports E without starting it. + ( + "log_details", + Box::new(|| assert_eq!(repo.log_details("HEAD").unwrap().signature, "E", "signed, never verified")), + ), + // Passes --no-show-signature itself, so it never ran the verifier. + ( + "file history", + Box::new(|| { + let query = crate::LogQuery { path: Some(file.clone().into()), ..Default::default() }; + assert!(!repo.log(&query).unwrap().is_empty(), "file history is empty"); + }), + ), + ( + "block_history", + Box::new(|| { + let history = + repo.block_history(&file, None, (0, 1), encoding_rs::UTF_8, &CancellationToken::new()).unwrap(); + assert!(!history.revisions.is_empty(), "block history is empty"); + }), + ), + ]; + let mut ran = Vec::new(); + for (name, call) in &calls { + let _ = std::fs::remove_file(&marker); + call(); + if marker.exists() { + ran.push(*name); + } + } + // Control: the fixture's verifier really runs when the same git is used directly. + let _ = std::fs::remove_file(&marker); + let git = cli::detected_git_executable().unwrap(); + let _ = Command::new(git).current_dir(&root).args(["log", "-1", "--format=%H"]).output(); + assert!(marker.exists(), "control: plain `git log` did not run gpg.program, fixture is not live"); + assert!(ran.is_empty(), "Restricted Mode ran the repository's gpg.program via: {ran:?}"); + } + + /// Security audit lead 2: turning signature display off is not enough. A git that is asked to + /// verify (`%G?`, `--show-signature`, or a `format.pretty` the repository sets) runs the + /// verifier of the signature's format (OpenPGP, SSH and X.509 each have a program key), so + /// Restricted Mode must leave none of them pointing at the repository's program. And when + /// nothing asks, the repository's `log.showSignature` must not make git look for one. + #[test] + fn restricted_git_does_not_execute_signature_verifier_when_asked_to_verify() { + let temp = tempfile::tempdir().unwrap(); + let root = normalized(temp.path()); + init(&root, "main"); + let (marker, script) = verifier_script(&root); + for key in ["gpg.program", "gpg.ssh.program", "gpg.x509.program"] { + git(&root, &["config", key, &script]); + } + // git starts the SSH verifier only when an allowed-signers file exists. + let signers = root.join(".git/allowed-signers"); + std::fs::write(&signers, "").unwrap(); + git(&root, &["config", "gpg.ssh.allowedSignersFile", &signers.to_str().unwrap().replace('\\', "/")]); + + git(&root, &["config", "format.pretty", "%H %G?"]); + git(&root, &["config", "log.showSignature", "true"]); + + let unasked: &[&str] = &["log", "-1", "--format=%H"]; + let requests: [&[&str]; 4] = [ + &["log", "-1", "--format=%G?"], + &["log", "-1", "--show-signature", "--format=%H"], + &["log", "-1"], + unasked, + ]; + let mut ran = Vec::new(); + for armor in ["PGP SIGNATURE", "SSH SIGNATURE", "SIGNED MESSAGE"] { + sign_head(&root, armor); + for args in requests { + for allowed in [false, true] { + let _ = std::fs::remove_file(&marker); + let out = { + let _scope = crate::ExecutionScope::enter(allowed); + cli::run(&root, args, None).unwrap() + }; + assert!(!out.stdout.is_empty(), "{armor} {args:?}: git printed nothing"); + if allowed { + // Control: the same request runs the fixture's verifier in a trusted project. + assert!(marker.exists(), "control: {armor} {args:?} did not run the verifier"); + } else if marker.exists() { + ran.push(format!("{armor} {args:?}")); + } else if args == unasked { + // A git that looked for a verifier and found none says so here. + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.is_empty(), "{armor} {args:?}: git looked for a verifier: {stderr}"); + } + } + } + } + assert!(ran.is_empty(), "Restricted Mode ran the repository's signature verifier via: {ran:?}"); + } + + /// A repository with a signed HEAD whose config has `git log` verify it with a program of + /// its own. Returns the marker that program creates. + fn signed_log_fixture(root: &Path) -> PathBuf { + init(root, "main"); + git(root, &["config", "commit.gpgsign", "false"]); + write(root, "src/move.txt", "move\nsigned change\n"); + git(root, &["commit", "-qam", "signed"]); + sign_head(root, "PGP SIGNATURE"); + let (marker, script) = verifier_script(root); + git(root, &["config", "log.showSignature", "true"]); + git(root, &["config", "gpg.program", &script]); + marker + } + + /// Security audit lead 2: a repository nobody has vouched for is restricted. Execution is + /// something a caller grants, not something it has to remember to take away. + #[test] + fn discovered_repository_runs_no_repository_program_until_execution_is_granted() { + let temp = tempfile::tempdir().unwrap(); + let root = normalized(temp.path()); + let marker = signed_log_fixture(&root); + + let repo = crate::Repo::discover(&root).unwrap().unwrap(); + let users = repo.log_users().unwrap(); + assert!(!marker.exists(), "a freshly discovered repository ran its gpg.program"); + assert_eq!(users, ["Test "]); + + // Control: the same call runs the fixture's verifier once execution is granted. + repo.set_execution_allowed(true); + repo.log_users().unwrap(); + assert!(marker.exists(), "control: granting execution did not run gpg.program, fixture is not live"); + } + + /// Security audit lead 2: status is read through gitoxide, which runs the clean filter of + /// the repository's config to compare a touched file with the index. A repository nobody + /// has vouched for is opened without that config. + #[test] + fn discovered_repository_status_runs_no_repository_filter_until_execution_is_granted() { + let temp = tempfile::tempdir().unwrap(); + let root = normalized(temp.path()); + init(&root, "main"); + let marker = root.join("filter-marker"); + let clean = format!("echo x > '{}'; cat", marker.to_str().unwrap().replace('\\', "/")); + git(&root, &["config", "filter.marker.clean", &clean]); + write(&root, ".gitattributes", "*.txt filter=marker\n"); + // Same length as the committed "move\n": only reading the content tells them apart. + write(&root, "src/move.txt", "MOVE\n"); + + let repo = crate::Repo::discover(&root).unwrap().unwrap(); + let status = repo.status(); + assert!(!marker.exists(), "a freshly discovered repository ran its clean filter"); + assert!(status.unwrap().iter().any(|f| f.path.0.ends_with("move.txt")), "the touched file is not reported"); + + // Control: the same call runs the fixture's filter once execution is granted. + repo.set_execution_allowed(true); + repo.status().unwrap(); + assert!(marker.exists(), "control: granting execution did not run the clean filter, fixture is not live"); + } + + /// Security audit lead 2: git started by code that declared no mode at all is restricted. + #[test] + fn git_started_outside_an_execution_scope_is_restricted() { + let temp = tempfile::tempdir().unwrap(); + let root = normalized(temp.path()); + let marker = signed_log_fixture(&root); + + assert!(!cli::run(&root, &["log", "-1", "--format=%H"], None).unwrap().stdout.is_empty()); + assert!(!marker.exists(), "git outside an execution scope ran the repository's gpg.program"); + + // Control: the same request runs the fixture's verifier inside a scope that allows it. + { + let _scope = crate::ExecutionScope::enter(true); + cli::run(&root, &["log", "-1", "--format=%H"], None).unwrap(); + } + assert!(marker.exists(), "control: an allowing scope did not run gpg.program, fixture is not live"); + } + + /// Security audit lead 2: git fetches an object it misses from a partial clone's promisor, + /// running the `uploadpack` program the repository names. Listing a revision's directory in + /// Restricted Mode must not. + #[test] + fn restricted_revision_listing_does_not_run_the_promisor_of_the_repository() { + let temp = tempfile::tempdir().unwrap(); + let root = normalized(temp.path()); + init(&root, "main"); + let marker = root.join("lazy-fetch-marker"); + let spelled = |p: &Path| p.to_str().unwrap().replace('\\', "/"); + let out = Command::new("git").current_dir(&root).args(["rev-parse", "HEAD^{tree}"]).output().unwrap(); + let tree = String::from_utf8(out.stdout).unwrap().trim().to_owned(); + git(&root, &["config", "extensions.partialClone", "origin"]); + git(&root, &["config", "remote.origin.url", &spelled(&root)]); + git(&root, &["config", "remote.origin.promisor", "true"]); + git(&root, &["config", "remote.origin.partialclonefilter", "blob:none"]); + git( + &root, + &["config", "remote.origin.uploadpack", &format!("echo ran > '{}'; git-upload-pack", spelled(&marker))], + ); + // The listing needs HEAD's tree; with it gone, git asks the promisor for it. + std::fs::remove_file(root.join(".git/objects").join(&tree[..2]).join(&tree[2..])).unwrap(); + + let repo = crate::Repo::discover(&root).unwrap().unwrap(); + repo.set_execution_allowed(false); + assert!(repo.revision_children("HEAD", "").is_err(), "the tree is missing and must stay missing"); + assert!(!marker.exists(), "Restricted Mode ran the repository's uploadpack"); + + // Control: in a trusted project the same listing asks the promisor. + repo.set_execution_allowed(true); + let _ = repo.revision_children("HEAD", ""); + assert!(marker.exists(), "control: a trusted listing did not run uploadpack, fixture is not live"); + } } diff --git a/crates/ide-vcs/src/lib.rs b/crates/ide-vcs/src/lib.rs index 5e21ce704..fd42c46d5 100644 --- a/crates/ide-vcs/src/lib.rs +++ b/crates/ide-vcs/src/lib.rs @@ -638,8 +638,8 @@ mod text_encoding; mod write; pub use cli::{ - GitCommandObserver, detected_git_executable, git_version, set_askpass_env, set_git_command_observer, - set_git_executable, + GitCommandObserver, detected_git_executable, filter_overrides_from, git_version, restrict, set_askpass_env, + set_git_command_observer, set_git_executable, }; pub use diff::build_partial_patch; pub use ignore::{IgnoreDestination, add_to_ignore}; @@ -674,6 +674,8 @@ pub fn clone( let dest = dest.to_string_lossy(); // Cloning must not inherit an enclosing project's executable Git settings. let cwd = tempfile::tempdir()?; + // No repository's config applies in that empty folder, so the user's git runs as it is. + let _scope = ExecutionScope::enter(true); let args = clone_args(url, &dest, depth)?; let args: Vec<&str> = args.iter().map(String::as_str).collect(); cli::run_streaming(cwd.path(), &args, cancel, on_output) @@ -731,7 +733,8 @@ pub struct Repo { } impl Repo { - /// Discovers the repository containing `path`; `Ok(None)` when not under git. + /// Discovers the repository containing `path`; `Ok(None)` when not under git. The + /// repository is in Restricted Mode until [`Repo::set_execution_allowed`] says otherwise. pub fn discover(path: &Path) -> IdeResult> { repo::discover(path) } @@ -986,6 +989,7 @@ impl Repo { /// Lists one immutable revision directory. Directory names end in `/`. pub fn revision_children(&self, revision: &str, directory: &str) -> IdeResult> { + let _scope = crate::ExecutionScope::enter(self.execution_allowed.load(std::sync::atomic::Ordering::Acquire)); let repo = self.gix()?; let id = repo::resolve(&repo, revision)?.to_string(); let directory = self.rel_path(std::path::Path::new(directory))?; @@ -1056,7 +1060,9 @@ impl Repo { let rel = path.map(|p| self.rel_path(p)).transpose()?; let overrides = cli::filter_overrides(self.root(), cancel)?; let mut args: Vec<&str> = overrides.iter().map(String::as_str).collect(); - args.extend(["--literal-pathspecs", "--no-pager", "diff", "--no-ext-diff", "--no-textconv", rev, "--"]); + args.extend(["--literal-pathspecs", "--no-pager", "diff", "--no-ext-diff", "--no-textconv"]); + args.extend_from_slice(cli::submodule_guards()); + args.extend([rev, "--"]); if let Some(rel) = &rel { args.push(rel); } @@ -1175,11 +1181,13 @@ impl Repo { /// Edit Commit Message without touching the index or the working tree; returns HEAD /// before and after. pub fn reword(&self, commit: &str, message: &str) -> IdeResult { + let _scope = crate::ExecutionScope::enter(self.execution_allowed.load(std::sync::atomic::Ordering::Acquire)); write::reword(self, commit, message).map(|(before, after)| HeadChange { before, after }) } /// Undoes a history rewrite: HEAD goes from `after` back to `before`, local changes kept. pub fn restore_head(&self, before: &str, after: &str) -> IdeResult<()> { + let _scope = crate::ExecutionScope::enter(self.execution_allowed.load(std::sync::atomic::Ordering::Acquire)); write::restore_head(self, before, after) } @@ -1301,6 +1309,7 @@ impl Repo { /// Whether the repository has an executable hook `name` (e.g. `pre-push`), wherever /// `core.hooksPath` puts it: the Push dialog then offers to skip it. pub fn has_hook(&self, name: &str) -> IdeResult { + let _scope = crate::ExecutionScope::enter(self.execution_allowed.load(std::sync::atomic::Ordering::Acquire)); write::has_hook(self, name) } /// Pushes a local branch, or its history up to `revision`, to its upstream (Git Log @@ -1551,11 +1560,17 @@ impl Repo { } } -thread_local! { static EXECUTION_ALLOWED: std::cell::Cell = const { std::cell::Cell::new(true) }; } -struct ExecutionScope(bool); +// `None`: nobody has decided yet. Git started now is restricted, and the first scope entered +// decides. The flag governs the git CLI only: gitoxide follows the repository's own flag. +thread_local! { static EXECUTION_ALLOWED: std::cell::Cell> = const { std::cell::Cell::new(None) }; } +struct ExecutionScope(Option); impl ExecutionScope { + /// The outermost scope decides; a scope inside it can only restrict. fn enter(allowed: bool) -> Self { - Self(EXECUTION_ALLOWED.with(|current| current.replace(current.get() && allowed))) + Self( + EXECUTION_ALLOWED + .with(|current| current.replace(Some(current.get().map_or(allowed, |outer| outer && allowed)))), + ) } } impl Drop for ExecutionScope { @@ -1564,9 +1579,12 @@ impl Drop for ExecutionScope { } } pub(crate) fn execution_allowed() -> bool { - EXECUTION_ALLOWED.with(|current| current.get()) + EXECUTION_ALLOWED.with(|current| current.get().unwrap_or(false)) } impl Repo { + /// Whether git may run what the repository's config names (hooks, filters, the fsmonitor, + /// signature verifiers, a promisor) and use the configured git executable. A repository + /// starts without it: only a caller that knows the project is trusted grants it. pub fn set_execution_allowed(&self, allowed: bool) { let _scope = crate::ExecutionScope::enter(self.execution_allowed.load(std::sync::atomic::Ordering::Acquire)); self.execution_allowed.store(allowed, std::sync::atomic::Ordering::Release); diff --git a/crates/ide-vcs/src/repo.rs b/crates/ide-vcs/src/repo.rs index 9c99c5ee2..571b2c4cc 100644 --- a/crates/ide-vcs/src/repo.rs +++ b/crates/ide-vcs/src/repo.rs @@ -71,7 +71,7 @@ pub(crate) fn discover(path: &Path) -> IdeResult> { let pack_dir = common_dir.join("objects").join("pack"); let cache = Cache { stamp: stamp(&files), files, repo: ts, pack_stamp: pack_stamp(&pack_dir), pack_dir }; Ok(Some(Repo { - execution_allowed: std::sync::atomic::AtomicBool::new(true), + execution_allowed: std::sync::atomic::AtomicBool::new(false), root, git_dir, inner: std::sync::Mutex::new(cache), diff --git a/crates/ide-vcs/src/shelf.rs b/crates/ide-vcs/src/shelf.rs index cdd5f882f..3ba5c1bd6 100644 --- a/crates/ide-vcs/src/shelf.rs +++ b/crates/ide-vcs/src/shelf.rs @@ -200,6 +200,7 @@ pub(crate) fn unshelve(r: &Repo, shelf: &Path, id: &str, remove: bool) -> IdeRes } pub fn unshelve_path(r: &Repo, shelf: &Path, id: &str, remove: bool, path: Option<&str>) -> IdeResult { + let _scope = crate::ExecutionScope::enter(r.execution_allowed.load(std::sync::atomic::Ordering::Acquire)); let patch = selected_patch(shelf, id, path)?; let outcome = crate::patch::apply(r, &patch)?; if remove && outcome.conflicts.is_empty() { @@ -291,6 +292,7 @@ pub fn delete_path(shelf: &Path, id: &str, path: Option<&str>) -> IdeResult<()> /// Reconstruct each side in a throwaway index; neither HEAD nor the working files are used. pub fn diff_files(r: &Repo, shelf: &Path, id: &str) -> IdeResult> { + let _scope = crate::ExecutionScope::enter(r.execution_allowed.load(std::sync::atomic::Ordering::Acquire)); let patch = patch_of(shelf, id)?; let mut out = Vec::new(); for (section, file) in sections(&patch) { diff --git a/crates/ide-vcs/tests/block_history.rs b/crates/ide-vcs/tests/block_history.rs index 961282736..cad40209f 100644 --- a/crates/ide-vcs/tests/block_history.rs +++ b/crates/ide-vcs/tests/block_history.rs @@ -40,8 +40,11 @@ impl Fixture { self.git(&["add", "-A"]); self.git(&["commit", "-q", "-m", msg]); } + /// The repository of a trusted project. fn repo(&self) -> Repo { - Repo::discover(&self.dir).unwrap().expect("repository found") + let repo = Repo::discover(&self.dir).unwrap().expect("repository found"); + repo.set_execution_allowed(true); + repo } fn path(&self, rel: &str) -> PathBuf { self.dir.join(rel) diff --git a/crates/ide-vcs/tests/git_workflows.rs b/crates/ide-vcs/tests/git_workflows.rs index 226de37f1..3d59b60fb 100644 --- a/crates/ide-vcs/tests/git_workflows.rs +++ b/crates/ide-vcs/tests/git_workflows.rs @@ -103,8 +103,11 @@ impl Fixture { fn subjects(&self) -> Vec { self.git(&["log", "--format=%s"]).lines().map(str::to_string).collect() } + /// The repository of a trusted project. fn repo(&self) -> Repo { - Repo::discover(&self.dir).unwrap().expect("repository found") + let repo = Repo::discover(&self.dir).unwrap().expect("repository found"); + repo.set_execution_allowed(true); + repo } fn path(&self, rel: &str) -> PathBuf { self.dir.join(rel) diff --git a/crates/ide-vcs/tests/history_ops.rs b/crates/ide-vcs/tests/history_ops.rs index f2af0663e..cc55168eb 100644 --- a/crates/ide-vcs/tests/history_ops.rs +++ b/crates/ide-vcs/tests/history_ops.rs @@ -58,8 +58,11 @@ impl Fixture { self.git(&["commit", "-q", "-m", msg]); self.git(&["rev-parse", "HEAD"]) } + /// The repository of a trusted project. fn repo(&self) -> Repo { - Repo::discover(&self.dir).unwrap().expect("repository found") + let repo = Repo::discover(&self.dir).unwrap().expect("repository found"); + repo.set_execution_allowed(true); + repo } fn path(&self, rel: &str) -> PathBuf { self.dir.join(rel) diff --git a/crates/ide-vcs/tests/repo.rs b/crates/ide-vcs/tests/repo.rs index c6d9f1e4b..9535996bf 100644 --- a/crates/ide-vcs/tests/repo.rs +++ b/crates/ide-vcs/tests/repo.rs @@ -65,8 +65,11 @@ impl Fixture { self.git(&["commit", "-q", "-m", msg]); self.git(&["rev-parse", "HEAD"]) } + /// The repository of a trusted project. fn repo(&self) -> Repo { - Repo::discover(&self.dir).unwrap().expect("repository found") + let repo = Repo::discover(&self.dir).unwrap().expect("repository found"); + repo.set_execution_allowed(true); + repo } fn path(&self, rel: &str) -> PathBuf { self.dir.join(rel) @@ -724,6 +727,7 @@ fn file_at_in_a_blobless_clone() { let url = format!("file://{}", fx.dir.display()); git_in(fx.dir.parent().unwrap(), &["clone", "-q", "--filter=blob:none", &url, "clone"]); let repo = Repo::discover(&clone).unwrap().unwrap(); + repo.set_execution_allowed(true); // v1's blob was never fetched; reading it fetches it on demand. assert_eq!(repo.file_at(&clone.join("f.txt"), Some(&c1), encoding_rs::UTF_8).unwrap().as_deref(), Some("v1\n")); } diff --git a/crates/ide-vfs/src/bytes.rs b/crates/ide-vfs/src/bytes.rs index 128b1cef2..9a44ec9c5 100644 --- a/crates/ide-vfs/src/bytes.rs +++ b/crates/ide-vfs/src/bytes.rs @@ -97,8 +97,80 @@ pub fn image_mime(path: &Path) -> Option<&'static str> { }) } -/// The whole image as a `data:` URL (allowed by the webview's CSP, unlike file URLs). -pub fn read_data_url(path: &Path) -> IdeResult { +/// The server and share of a Windows network path (`\\server\share\…`, also written with `/` +/// or as `\\?\UNC\…`). +#[cfg(windows)] +fn share(path: &Path) -> Option<(&std::ffi::OsStr, &std::ffi::OsStr)> { + use std::path::{Component, Prefix}; + let mut components = path.components(); + let Component::Prefix(prefix) = components.next()? else { return None }; + match prefix.kind() { + // `//?/UNC/server/share`, the spelling a canonical share path crosses the IPC boundary + // in (`ide_core::ipc_path`): with `/`, `std` reads the share `UNC` of a server `?`. + Prefix::UNC(device, unc) if device == "?" => { + let names = (components.next()?, components.next()?, components.next()?); + match names { + (Component::RootDir, Component::Normal(server), Component::Normal(share)) + if unc.eq_ignore_ascii_case("UNC") => + { + Some((server, share)) + } + _ => None, + } + } + Prefix::UNC(server, share) | Prefix::VerbatimUNC(server, share) => Some((server, share)), + _ => None, + } +} + +/// Whether `path` names a file on one of this machine's drives. On Windows that is an absolute +/// path starting at a drive letter. Every other spelling can lead to a server, and opening a +/// share signs in to its server with the user's credentials: `\\server\share`, `\\?\UNC\…`, +/// device paths, and `\??\UNC\…`, which carries no prefix `std` knows and which Windows opens +/// as written. +pub fn on_local_drive(path: &Path) -> bool { + #[cfg(windows)] + { + use std::path::{Component, Prefix}; + path.is_absolute() + && matches!( + path.components().next(), + Some(Component::Prefix(prefix)) if matches!(prefix.kind(), Prefix::Disk(_) | Prefix::VerbatimDisk(_)) + ) + } + #[cfg(not(windows))] + { + let _ = path; + true + } +} + +/// Whether two network paths are on the same share of the same server. +pub fn same_share(a: &Path, b: &Path) -> bool { + #[cfg(windows)] + { + let same = |x: &std::ffi::OsStr, y: &std::ffi::OsStr| x.eq_ignore_ascii_case(y); + matches!((share(a), share(b)), (Some(a), Some(b)) if same(a.0, b.0) && same(a.1, b.1)) + } + #[cfg(not(windows))] + { + let _ = (a, b); + false + } +} + +/// Whether a path written in content (Markdown, a page, an agent's reply) may be opened: one +/// on a local drive, or on the share one of `opened` (what the user opened) is on. +pub fn content_path_allowed>(path: &Path, opened: &[P]) -> bool { + on_local_drive(path) || opened.iter().any(|o| same_share(o.as_ref(), path)) +} + +/// The whole image as a `data:` URL (allowed by the webview's CSP, unlike file URLs). The path +/// often comes from text someone else wrote (Markdown): see [`content_path_allowed`]. +pub fn read_data_url(path: &Path, opened: &[std::path::PathBuf]) -> IdeResult { + if !content_path_allowed(path, opened) { + return Err(IdeError::invalid(format!("{} is not on a local drive or an opened share", path.display()))); + } let mime = image_mime(path).ok_or_else(|| IdeError::invalid(format!("{} is not a supported image", path.display())))?; if let Some(bytes) = archive_bytes(path, MAX_IMAGE_SIZE)? { @@ -138,6 +210,114 @@ pub fn base64(bytes: &[u8]) -> String { mod tests { use super::*; + /// Security audit lead 3: an image path from agent markdown must not make the host open a + /// network share (on Windows that authenticates to the named host with the user's credentials). + #[cfg(windows)] + #[test] + fn read_data_url_refuses_network_paths() { + let dir = tempfile::tempdir().unwrap(); + let local = dir.path().join("dummy.png"); + std::fs::write(&local, b"\x89PNG dummy").unwrap(); + // The same file reached over SMB through the loopback admin share. + let s = local.to_str().unwrap(); + let back = format!(r"\\127.0.0.1\{}${}", &s[..1], &s[2..]); + // The webview hands the host the forward-slash form (`file://host/share` -> `//host/share`). + let forward = back.replace('\\', "/"); + // The NT spelling: no `\\` in front, and Windows opens it all the same. + let nt = format!(r"\??\UNC{}", &back[1..]); + for unc in [forward, back, nt].map(std::path::PathBuf::from) { + assert!( + std::fs::read(&unc).is_ok(), + "control: {} is not reachable over SMB, fixture not live", + unc.display() + ); + let read = read_data_url(&unc, &[]); + assert!(read.is_err(), "read_data_url opened {} over SMB: {:?}", unc.display(), read.map(|s| s.len())); + } + } + + /// The exception to the above: an image on the share a project is open on, and only there. + #[cfg(windows)] + #[test] + fn read_data_url_reads_the_share_a_project_is_on() { + let dir = tempfile::tempdir().unwrap(); + let local = dir.path().join("dummy.png"); + std::fs::write(&local, b"\x89PNG dummy").unwrap(); + let s = local.to_str().unwrap(); + let on = |server: &str| std::path::PathBuf::from(format!(r"\\{server}\{}${}", &s[..1], &s[2..])); + let image = on("127.0.0.1"); + let wanted = read_data_url(&local, &[]).unwrap(); + // A project root as typed, and as `canonicalize` returns it. + for root in [r"\\127.0.0.1\C$\work", r"\\?\UNC\127.0.0.1\c$\work"].map(std::path::PathBuf::from) { + let opened = [std::path::PathBuf::from(r"D:\other"), root]; + assert_eq!(read_data_url(&image, &opened).unwrap(), wanted); + assert_eq!(read_data_url(Path::new(&image.to_str().unwrap().replace('\\', "/")), &opened).unwrap(), wanted); + // The spelling paths of a project on a share cross the IPC boundary in. + let wire = |path: &Path| std::path::PathBuf::from(format!("//?/UNC/{}", &path.to_str().unwrap()[2..])); + assert_eq!(read_data_url(&wire(&image), &opened).unwrap(), wanted); + assert_eq!(read_data_url(&image, &[wire(Path::new(r"\\127.0.0.1\C$\work"))]).unwrap(), wanted); + // The same file through another name of this machine: a server the user did not open. + let other = on("localhost"); + assert!(std::fs::read(&other).is_ok(), "control: {} is not reachable over SMB", other.display()); + for other in [wire(&other), other] { + let read = read_data_url(&other, &opened); + assert!( + read.is_err(), + "read_data_url opened {} over SMB: {:?}", + other.display(), + read.map(|s| s.len()) + ); + } + } + } + + #[cfg(windows)] + #[test] + fn only_absolute_paths_starting_at_a_drive_letter_are_local() { + for path in [r"C:\a.png", "C:/a.png", r"\\?\C:\a.png", r"z:\share\a.png"] { + assert!(on_local_drive(Path::new(path)), "{path}"); + } + for path in [ + r"\\server\share\a.png", + "//server/share/a.png", + r"/\server/share/a.png", + r"\\?\UNC\server\share\a.png", + "//?/UNC/server/share/a.png", + r"\\.\UNC\server\share\a.png", + r"\\?\GLOBALROOT\Device\Mup\server\share\a.png", + r"\??\UNC\server\share\a.png", + r"\??\C:\a.png", + r"\a.png", + r"C:a.png", + "a.png", + r"..\a.png", + ] { + assert!(!on_local_drive(Path::new(path)), "{path}"); + } + let project = Path::new(r"\\nas\work\app"); + assert!(same_share(project, Path::new("//NAS/Work/app/docs/a.png"))); + assert!(same_share(project, Path::new(r"\\?\UNC\nas\work\other\a.png"))); + assert!(!same_share(project, Path::new(r"\\nas\private\a.png"))); + assert!(!same_share(project, Path::new(r"\\evil\work\app\a.png"))); + assert!(!same_share(project, Path::new(r"\??\UNC\nas\work\app\a.png"))); + assert!(!same_share(Path::new(r"C:\app"), Path::new(r"C:\app\a.png"))); + // Read by `std` as the share `UNC` of a server named `?`: the spelling a share's paths + // cross the IPC boundary in. + assert!(!same_share(Path::new("//?/UNC/nas/work/app"), Path::new("//?/UNC/evil/share/a.png"))); + assert!(same_share(Path::new("//?/UNC/nas/work/app"), Path::new("//?/unc/NAS/Work/docs/a.png"))); + assert!(same_share(project, Path::new("//?/UNC/nas/work/docs/a.png"))); + assert!(same_share(Path::new(r"\\?\UNC\nas\work\app"), Path::new("//?/UNC/nas/work/docs/a.png"))); + for device in ["//?/UNC/nas", "//?/UNC/../nas/work/a.png", "//?/C:/a.png", "//?/nas/work/a.png"] { + assert!(!same_share(Path::new(device), Path::new(device)), "{device}"); + } + + assert!(content_path_allowed(Path::new(r"C:\app\a.png"), &[project])); + assert!(content_path_allowed(Path::new(r"\\nas\work\docs\a.png"), &[Path::new(r"C:\app"), project])); + assert!(!content_path_allowed(Path::new(r"\\nas\private\a.png"), &[project])); + assert!(!content_path_allowed(Path::new(r"\\nas\work\docs\a.png"), &[Path::new(r"C:\app")])); + assert!(!content_path_allowed(Path::new(r"\\nas\work\docs\a.png"), &[] as &[&Path])); + } + #[test] fn base64_matches_rfc4648_vectors() { for (input, expected) in [ @@ -190,12 +370,12 @@ mod tests { let png = dir.path().join("x.PNG"); fs::write(&png, [0x89, b'P', b'N', b'G']).unwrap(); assert_eq!( - read_data_url(&png).unwrap(), + read_data_url(&png, &[]).unwrap(), format!("data:image/png;base64,{}", base64(&[0x89, b'P', b'N', b'G'])) ); let txt = dir.path().join("x.txt"); fs::write(&txt, "x").unwrap(); - assert!(read_data_url(&txt).is_err()); + assert!(read_data_url(&txt, &[]).is_err()); assert_eq!(image_mime(Path::new("a/b.svg")), Some("image/svg+xml")); } } diff --git a/crates/ide-vfs/src/lib.rs b/crates/ide-vfs/src/lib.rs index bcdc71383..b7d8320fc 100644 --- a/crates/ide-vfs/src/lib.rs +++ b/crates/ide-vfs/src/lib.rs @@ -21,7 +21,10 @@ pub use archive::{ ARCHIVE_EXTENSIONS, archive_entry_path, archive_root, is_archive_file, is_archive_path, read_archive_dir, read_archive_entry, split_archive_path, }; -pub use bytes::{ByteRange, FileProbe, MAX_RANGE_LEN, base64, image_mime, probe, read_data_url, read_range}; +pub use bytes::{ + ByteRange, FileProbe, MAX_RANGE_LEN, base64, content_path_allowed, image_mime, on_local_drive, probe, + read_data_url, read_range, same_share, +}; pub use compare::{ DirectoryCompareEntry, DirectoryCompareProgress, DirectoryCompareStatus, compare_directories, copy_merge, }; diff --git a/crates/ide-web/Cargo.toml b/crates/ide-web/Cargo.toml index e7130c2c0..f203328c4 100644 --- a/crates/ide-web/Cargo.toml +++ b/crates/ide-web/Cargo.toml @@ -7,8 +7,8 @@ license.workspace = true [dependencies] ide-core.workspace = true +dirs.workspace = true open = "5" -parking_lot.workspace = true percent-encoding = "2" serde.workspace = true specta.workspace = true diff --git a/crates/ide-web/src/lib.rs b/crates/ide-web/src/lib.rs index 15d8cc927..3ccf66b43 100644 --- a/crates/ide-web/src/lib.rs +++ b/crates/ide-web/src/lib.rs @@ -2,10 +2,13 @@ //! //! [`WebServer`] is the built-in server: it serves project files read-only on //! `127.0.0.1`, so pages opened from the IDE load relative scripts, styles and -//! ES modules the way they would from a real server. Every URL starts with a -//! random per-run token, and requests whose `Host` is not the server itself are -//! refused (DNS rebinding). [`installed`] lists the browsers found on this -//! machine and [`open`] launches one, or the system default. +//! ES modules the way they would from a real server. Each served root has a +//! port, and so an origin, of its own, and its URLs start with a random token +//! of its own, new on every run. A file outside the project is served with +//! what lies below its folder, dot names left out, or alone when that folder +//! is the home folder or holds it. Requests whose `Host` is not the server +//! itself are refused (DNS rebinding). [`installed`] lists the browsers found +//! on this machine and [`open`] launches one, or the system default. mod browsers; mod server; diff --git a/crates/ide-web/src/server.rs b/crates/ide-web/src/server.rs index c4c67cfc3..ef74961d1 100644 --- a/crates/ide-web/src/server.rs +++ b/crates/ide-web/src/server.rs @@ -1,5 +1,4 @@ use ide_core::{IdeError, IdeResult}; -use parking_lot::RwLock; use percent_encoding::{AsciiSet, CONTROLS, percent_decode_str, utf8_percent_encode}; use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -8,8 +7,6 @@ use std::time::Duration; use tokio::io::{AsyncBufReadExt, AsyncReadExt, AsyncWriteExt, BufReader}; use tokio::net::{TcpListener, TcpStream}; -/// IDEA's built-in server port, tried first so bookmarked URLs keep working. -const PREFERRED_PORT: u16 = 63342; const MAX_HEAD: usize = 16 * 1024; const READ_TIMEOUT: Duration = Duration::from_secs(10); @@ -32,16 +29,41 @@ const SEGMENT: &AsciiSet = &CONTROLS .add(b'|') .add(b'}'); -struct Shared { +/// A served directory, at `http://127.0.0.1:///…`. The port and the token +/// are its own. The port makes it another origin to a browser, which does not let a page of +/// one root read a response of another. The token keeps such a page from naming a file of +/// another root at all: a port can be found by scanning and a key guessed, a token cannot. +/// +/// Storage is not isolated for good: cookies are per host, not per port, and a port may +/// serve another root on a later run. +struct Root { + key: String, token: String, - /// URL key (`///…`) → served directory. - roots: RwLock>, + dir: PathBuf, + port: u16, + serves: Serves, } -/// The built-in web server; started on the first [`WebServer::url_for`]. +/// What of its directory a root serves. +#[derive(PartialEq)] +enum Serves { + /// A project: all of it. + All, + /// Everything below the folder of a file outside the project, so the page loads its + /// scripts and styles, except what has a dot name on the way to it (`.ssh`, `.env`). + Folder, + /// This one file outside the project, resolved. Its folder is the home folder or lies + /// above it, where credentials have names anyone knows, with a dot or without + /// (`AppData`, `Library`); or that cannot be told; or its own name starts with a dot. + File(PathBuf), +} + +/// The built-in web server; a root starts being served on the first [`WebServer::url_for`] in it. pub struct WebServer { - shared: Arc, - port: tokio::sync::OnceCell, + /// URL key → what is served under it. + roots: tokio::sync::Mutex>>, + /// The home folder, resolved. + home: Option, } impl Default for WebServer { @@ -52,23 +74,30 @@ impl Default for WebServer { impl WebServer { pub fn new() -> Self { - Self { - shared: Arc::new(Shared { token: uuid::Uuid::new_v4().simple().to_string(), roots: RwLock::default() }), - port: tokio::sync::OnceCell::new(), + let home = dirs::home_dir().and_then(|home| std::fs::canonicalize(home).ok()); + if home.is_none() { + tracing::warn!("built-in web server: no home folder, so files outside a project are served alone"); } + Self { roots: Default::default(), home } } /// The URL serving `file`: under `root` (the project) when it contains the /// file, else under the file's directory. Starts the server if needed. + /// + /// A project that is the home folder, or holds it, is no boundary to serve whole: its + /// files are served like files outside a project. pub async fn url_for(&self, root: Option<&Path>, file: &Path) -> IdeResult { - let base = match root { - Some(r) if file.starts_with(r) && file != r => r, - _ => file.parent().ok_or_else(|| IdeError::invalid(format!("no directory: {}", file.display())))?, + let (base, serves) = match root { + Some(r) if file.starts_with(r) && file != r && self.holds_home(r).await != Some(true) => (r, Serves::All), + _ => { + let dir = + file.parent().ok_or_else(|| IdeError::invalid(format!("no directory: {}", file.display())))?; + (dir, self.outside(dir, file).await?) + } }; let rel = file.strip_prefix(base).map_err(|e| IdeError::internal(e.to_string()))?; - let port = self.port().await?; - let key = self.register(base); - let mut url = format!("http://127.0.0.1:{port}/{}/{}", self.shared.token, enc(&key)); + let root = self.root(base, serves).await?; + let mut url = format!("http://127.0.0.1:{}/{}/{}", root.port, root.token, enc(&root.key)); for c in rel.components() { url.push('/'); url.push_str(&enc(&c.as_os_str().to_string_lossy())); @@ -76,11 +105,29 @@ impl WebServer { Ok(url) } - /// The key `dir` is served under: its name, made unique. - fn register(&self, dir: &Path) -> String { - let mut roots = self.shared.roots.write(); - if let Some((k, _)) = roots.iter().find(|(_, d)| d.as_path() == dir) { - return k.clone(); + /// Whether `dir` is the home folder or lies above it; `None` when that cannot be told. + async fn holds_home(&self, dir: &Path) -> Option { + let dir = tokio::fs::canonicalize(dir).await.ok()?; + Some(self.home.as_ref()?.starts_with(dir)) + } + + /// How much of `dir` is served for `file` in it, a file outside the project: the folder + /// only when it is known not to hold the home folder. + async fn outside(&self, dir: &Path, file: &Path) -> IdeResult { + let dot_file = file.file_name().is_some_and(|name| name.to_string_lossy().starts_with('.')); + if self.holds_home(dir).await == Some(false) && !dot_file { + return Ok(Serves::Folder); + } + let resolved = tokio::fs::canonicalize(file).await; + Ok(Serves::File(resolved.map_err(|e| IdeError::invalid(format!("cannot serve {}: {e}", file.display())))?)) + } + + /// `dir` as a served root, listening from now on if it was not yet. Its key is its name, + /// made unique. + async fn root(&self, dir: &Path, serves: Serves) -> IdeResult> { + let mut roots = self.roots.lock().await; + if let Some(root) = roots.values().find(|r| r.dir == dir && r.serves == serves) { + return Ok(root.clone()); } let name: String = dir .file_name() @@ -94,43 +141,42 @@ impl WebServer { .map(|i| if i == 1 { name.clone() } else { format!("{name}-{i}") }) .find(|k| !roots.contains_key(k)) .expect("unbounded"); - roots.insert(key.clone(), dir.to_path_buf()); - key + // No fixed port: it would hand the browser storage of whichever root took it on one + // run to whichever takes it on the next. + let unavailable = + |e: std::io::Error| IdeError::unavailable(format!("cannot start the built-in web server: {e}")); + let listener = TcpListener::bind(("127.0.0.1", 0)).await.map_err(unavailable)?; + let port = listener.local_addr().map_err(unavailable)?.port(); + let root = Arc::new(Root { + key: key.clone(), + token: uuid::Uuid::new_v4().simple().to_string(), + dir: dir.to_path_buf(), + port, + serves, + }); + tokio::spawn(accept(listener, root.clone())); + tracing::info!("built-in web server on 127.0.0.1:{} for {}", root.port, dir.display()); + roots.insert(key, root.clone()); + Ok(root) } +} - async fn port(&self) -> IdeResult { - self.port - .get_or_try_init(|| async { - let listener = match TcpListener::bind(("127.0.0.1", PREFERRED_PORT)).await { - Ok(l) => l, - Err(_) => TcpListener::bind(("127.0.0.1", 0)).await?, - }; - let port = listener.local_addr()?.port(); - let shared = self.shared.clone(); +async fn accept(listener: TcpListener, root: Arc) { + loop { + match listener.accept().await { + Ok((sock, _)) => { + let root = root.clone(); tokio::spawn(async move { - loop { - match listener.accept().await { - Ok((sock, _)) => { - let shared = shared.clone(); - tokio::spawn(async move { - if let Err(e) = serve(sock, &shared, port).await { - tracing::debug!("web server: {e}"); - } - }); - } - Err(e) => { - tracing::warn!("web server accept: {e}"); - tokio::time::sleep(Duration::from_millis(100)).await; - } - } + if let Err(e) = serve(sock, &root).await { + tracing::debug!("web server: {e}"); } }); - tracing::info!("built-in web server on 127.0.0.1:{port}"); - Ok::<_, std::io::Error>(port) - }) - .await - .copied() - .map_err(|e| IdeError::unavailable(format!("cannot start the built-in web server: {e}"))) + } + Err(e) => { + tracing::warn!("web server accept: {e}"); + tokio::time::sleep(Duration::from_millis(100)).await; + } + } } } @@ -183,16 +229,16 @@ enum Response { Error(u16, &'static str), } -async fn serve(mut sock: TcpStream, shared: &Shared, port: u16) -> std::io::Result<()> { +async fn serve(mut sock: TcpStream, root: &Root) -> std::io::Result<()> { let Ok(head) = tokio::time::timeout(READ_TIMEOUT, read_head(&mut sock)).await else { return Ok(()) }; let Some(req) = head? else { return Ok(()) }; let head_only = req.method == "HEAD"; let response = if !matches!(req.method.as_str(), "GET" | "HEAD") { Response::Error(405, "Method Not Allowed") - } else if !host_allowed(req.host.as_deref(), port) { + } else if !host_allowed(req.host.as_deref(), root.port) { Response::Error(403, "Forbidden") } else { - resolve(shared, &req.target).await + resolve(root, &req.target).await }; let mut out = String::new(); match &response { @@ -210,7 +256,10 @@ async fn serve(mut sock: TcpStream, shared: &Shared, port: u16) -> std::io::Resu )); } } - out.push_str("Cache-Control: no-store\r\nX-Content-Type-Options: nosniff\r\nConnection: close\r\n\r\n"); + out.push_str("Cache-Control: no-store\r\nX-Content-Type-Options: nosniff\r\n"); + // A page of another origin (another root's too) cannot load what is served here as a + // script, style or image. Browsers do not apply this to frames or navigations. + out.push_str("Cross-Origin-Resource-Policy: same-origin\r\nConnection: close\r\n\r\n"); sock.write_all(out.as_bytes()).await?; if !head_only { match response { @@ -229,18 +278,20 @@ fn host_allowed(host: Option<&str>, port: u16) -> bool { host.is_some_and(|h| h == format!("127.0.0.1:{port}") || h == format!("localhost:{port}")) } -async fn resolve(shared: &Shared, target: &str) -> Response { +async fn resolve(root: &Root, target: &str) -> Response { const NOT_FOUND: Response = Response::Error(404, "Not Found"); let path = target.split(['?', '#']).next().unwrap_or(""); let Some(rest) = path.strip_prefix('/') else { return NOT_FOUND }; let mut segments = rest.split('/'); - if segments.next() != Some(shared.token.as_str()) { + // A listener answers for its own root only, to its own token only. + if segments.next() != Some(root.token.as_str()) { return NOT_FOUND; } - let Some(key) = segments.next().and_then(dec) else { return NOT_FOUND }; - let Some(root) = shared.roots.read().get(&key).cloned() else { return NOT_FOUND }; - let Ok(real_root) = tokio::fs::canonicalize(&root).await else { return NOT_FOUND }; - let mut file = root; + if segments.next().and_then(dec).as_deref() != Some(root.key.as_str()) { + return NOT_FOUND; + } + let Ok(real_root) = tokio::fs::canonicalize(&root.dir).await else { return NOT_FOUND }; + let mut file = root.dir.clone(); // `//` names a directory too: it redirects like any other. let trailing_slash = path.ends_with('/'); for seg in segments { @@ -252,7 +303,7 @@ async fn resolve(shared: &Shared, target: &str) -> Response { _ => return NOT_FOUND, } } - let Some(real) = confined(&real_root, &file).await else { return NOT_FOUND }; + let Some(real) = confined(root, &real_root, &file).await else { return NOT_FOUND }; let Ok(meta) = tokio::fs::metadata(&real).await else { return NOT_FOUND }; if meta.is_dir() { // Relative URLs in an index page resolve against the directory. @@ -260,7 +311,7 @@ async fn resolve(shared: &Shared, target: &str) -> Response { return Response::Redirect(format!("{path}/")); } let index = file.join("index.html"); - let Some(real_index) = confined(&real_root, &index).await else { return NOT_FOUND }; + let Some(real_index) = confined(root, &real_root, &index).await else { return NOT_FOUND }; return match tokio::fs::metadata(&real_index).await { Ok(m) if m.is_file() => Response::File { path: real_index, len: m.len(), mime: content_type(&index) }, _ => NOT_FOUND, @@ -269,12 +320,20 @@ async fn resolve(shared: &Shared, target: &str) -> Response { Response::File { path: real, len: meta.len(), mime: content_type(&file) } } -/// `path` with every symlink resolved, if it still lies inside `real_root` (already -/// canonical). Rejecting `..` segments is not enough: a project can commit symlinks -/// that point anywhere on disk. -async fn confined(real_root: &Path, path: &Path) -> Option { +/// `path` with every symlink resolved, if `root` serves it: it lies inside `real_root` +/// (already canonical) and is within what the root serves. Rejecting `..` segments is not +/// enough: a project can commit symlinks that point anywhere on disk. A dot is looked for +/// in the resolved path because a link or a Windows short name (`SSH~1`) names a dot folder +/// without the dot. +async fn confined(root: &Root, real_root: &Path, path: &Path) -> Option { let real = tokio::fs::canonicalize(path).await.ok()?; - real.starts_with(real_root).then_some(real) + let below = real.strip_prefix(real_root).ok()?; + let served = match &root.serves { + Serves::All => true, + Serves::Folder => !below.components().any(|c| c.as_os_str().to_string_lossy().starts_with('.')), + Serves::File(file) => real == *file, + }; + served.then_some(real) } fn dec(s: &str) -> Option { @@ -317,6 +376,11 @@ fn content_type(path: &Path) -> &'static str { mod tests { use super::*; + /// A server whose home folder is nowhere near what a test serves. + fn server() -> WebServer { + WebServer { home: Some("no-such-home".into()), ..WebServer::new() } + } + async fn get(url: &str, host: Option<&str>) -> String { let rest = url.strip_prefix("http://").unwrap(); let (authority, path) = rest.split_at(rest.find('/').unwrap()); @@ -356,7 +420,8 @@ mod tests { assert!(get(&format!("{base}/%2e%2e/secret.txt"), None).await.starts_with("HTTP/1.1 404")); assert!(get(&format!("{base}/web/..%2f..%2fsecret.txt"), None).await.starts_with("HTTP/1.1 404")); // Wrong token, foreign Host (DNS rebinding). - let other = url.replacen(&server.shared.token, "0123", 1); + let token = url.split('/').nth(3).unwrap(); + let other = url.replacen(token, "0123", 1); assert!(get(&other, None).await.starts_with("HTTP/1.1 404")); assert!(get(&url, Some("evil.example:80")).await.starts_with("HTTP/1.1 403")); } @@ -406,10 +471,278 @@ mod tests { let (a, b) = (dir.path().join("x/site"), dir.path().join("y/site")); std::fs::create_dir_all(&a).unwrap(); std::fs::create_dir_all(&b).unwrap(); - let server = WebServer::new(); + let server = server(); let ua = server.url_for(None, &a.join("p.html")).await.unwrap(); let ub = server.url_for(Some(&a), &b.join("p.html")).await.unwrap(); assert!(ua.ends_with("/site/p.html") && ub.ends_with("/site-2/p.html"), "{ua} {ub}"); assert_eq!(server.url_for(None, &a.join("q.html")).await.unwrap(), ua.replace("p.html", "q.html")); } + + /// Security audit lead 1: a page served from one root must not read another root. + #[tokio::test] + async fn a_page_in_one_root_cannot_read_another_root() { + let dir = tempfile::tempdir().unwrap(); + let (untrusted, victim) = (dir.path().join("untrusted"), dir.path().join("victim")); + std::fs::create_dir_all(&untrusted).unwrap(); + std::fs::create_dir_all(&victim).unwrap(); + std::fs::write(untrusted.join("index.html"), "").unwrap(); + std::fs::write(victim.join(".env"), "DUMMY_SECRET").unwrap(); + std::fs::write(victim.join("other.html"), "

victim

").unwrap(); + + let server = WebServer::new(); + let ua = server.url_for(Some(&untrusted), &untrusted.join("index.html")).await.unwrap(); + server.url_for(Some(&victim), &victim.join("other.html")).await.unwrap(); + // What a script in the untrusted page can compute from its own location. + let cross = ua.replace("/untrusted/index.html", "/victim/.env"); + let page = get(&cross, None).await; + assert!(!page.ends_with("DUMMY_SECRET"), "the untrusted root's origin and token read the victim root: {page}"); + assert!(page.starts_with("HTTP/1.1 404"), "{page}"); + } + + /// Security audit lead 1: the page can find the other root's port by scanning and guess its + /// key, the folder's name. The only token it has is its own. + #[tokio::test] + async fn a_page_cannot_ask_another_roots_listener_with_its_own_token() { + let dir = tempfile::tempdir().unwrap(); + let (untrusted, victim) = (dir.path().join("untrusted"), dir.path().join("victim")); + std::fs::create_dir_all(&untrusted).unwrap(); + std::fs::create_dir_all(&victim).unwrap(); + std::fs::write(untrusted.join("index.html"), "").unwrap(); + std::fs::write(victim.join(".env"), "DUMMY_SECRET").unwrap(); + std::fs::write(victim.join("other.html"), "

victim

").unwrap(); + + let server = WebServer::new(); + let ua = server.url_for(Some(&untrusted), &untrusted.join("index.html")).await.unwrap(); + let uv = server.url_for(Some(&victim), &victim.join("other.html")).await.unwrap(); + let token = |url: &str| url.split('/').nth(3).unwrap().to_string(); + let forged = uv.replacen(&token(&uv), &token(&ua), 1).replace("/other.html", "/.env"); + let page = get(&forged, None).await; + assert!(page.starts_with("HTTP/1.1 404"), "the victim's listener answered the untrusted root's token: {page}"); + // Its own token still opens it. + assert!(get(&uv.replace("/other.html", "/.env"), None).await.ends_with("DUMMY_SECRET")); + } + + /// Security audit lead 1: roots do not share an origin, which is what a browser isolates + /// (fetches, storage, service workers), and no other origin may load a served file as a + /// script, style or image. + #[tokio::test] + async fn each_root_is_its_own_origin_and_keeps_its_files_from_other_origins() { + let dir = tempfile::tempdir().unwrap(); + let (a, b) = (dir.path().join("a"), dir.path().join("b")); + std::fs::create_dir_all(&a).unwrap(); + std::fs::create_dir_all(&b).unwrap(); + std::fs::write(a.join("index.html"), "

a

").unwrap(); + std::fs::write(b.join("index.html"), "

b

").unwrap(); + let server = WebServer::new(); + let ua = server.url_for(Some(&a), &a.join("index.html")).await.unwrap(); + let ub = server.url_for(Some(&b), &b.join("index.html")).await.unwrap(); + let origin = |url: &str| url.split('/').take(3).collect::>().join("/"); + assert_ne!(origin(&ua), origin(&ub), "{ua} {ub}"); + for (url, body) in [(&ua, "

a

"), (&ub, "

b

")] { + let page = get(url, None).await; + assert!(page.starts_with("HTTP/1.1 200") && page.ends_with(body), "{page}"); + assert!(page.contains("Cross-Origin-Resource-Policy: same-origin\r\n"), "{page}"); + } + // The other root's listener does not answer for this one. + let swapped = ub.replace(&origin(&ub), &origin(&ua)); + assert!(get(&swapped, None).await.starts_with("HTTP/1.1 404")); + } + + /// Security audit lead 7: a file outside the project is served together with the whole + /// folder it lies in, so a page saved to the home folder reads the keys kept there. The + /// same page opened as `file://` reads nothing but itself. + #[tokio::test] + async fn a_page_outside_the_project_cannot_read_the_dot_files_of_its_folder() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path().join("home"); + std::fs::create_dir_all(home.join(".ssh")).unwrap(); + std::fs::write(home.join("saved.html"), "").unwrap(); + std::fs::write(home.join(".ssh/id_ed25519"), "DUMMY_KEY").unwrap(); + + let server = server(); + let url = server.url_for(None, &home.join("saved.html")).await.unwrap(); + // What a script in the page can compute from its own location. + let key = url.replace("/saved.html", "/.ssh/id_ed25519"); + let page = get(&key, None).await; + assert!(!page.ends_with("DUMMY_KEY"), "a page outside the project read a key next to it: {page}"); + assert!(page.starts_with("HTTP/1.1 404"), "{page}"); + // A hidden folder is not a directory to redirect into either, however it is spelled. + for spelling in ["/.ssh", "/.ssh/", "/%2essh/id_ed25519", "/.SSH/id_ed25519"] { + let page = get(&url.replace("/saved.html", spelling), None).await; + assert!(page.starts_with("HTTP/1.1 404"), "{spelling}: {page}"); + } + } + + /// A link names a dot folder without the dot: the resolved path is what counts. + #[tokio::test] + async fn a_link_does_not_reach_the_dot_files_of_a_folder_outside_the_project() { + let dir = tempfile::tempdir().unwrap(); + let folder = dir.path().join("folder"); + std::fs::create_dir_all(folder.join(".ssh")).unwrap(); + std::fs::write(folder.join("saved.html"), "").unwrap(); + std::fs::write(folder.join(".ssh/id_ed25519"), "DUMMY_KEY").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(".ssh", folder.join("keys")).unwrap(); + #[cfg(windows)] + { + // A junction: unlike a symlink, Windows lets any user make one. + let made = std::process::Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(folder.join("keys")) + .arg(folder.join(".ssh")) + .output() + .unwrap(); + assert!(made.status.success(), "{made:?}"); + } + assert_eq!(std::fs::read_to_string(folder.join("keys/id_ed25519")).unwrap(), "DUMMY_KEY"); + + let server = server(); + let url = server.url_for(None, &folder.join("saved.html")).await.unwrap(); + let page = get(&url.replace("/saved.html", "/keys/id_ed25519"), None).await; + assert!(page.starts_with("HTTP/1.1 404"), "{page}"); + } + + /// Security audit lead 7: under the home folder lie credentials whose names anyone knows + /// and that start with no dot (`AppData/Roaming/gcloud/credentials.db`, `Library/…`). + #[tokio::test] + async fn a_page_in_the_home_folder_or_above_it_is_served_alone() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path().join("home"); + std::fs::create_dir_all(home.join("AppData/Roaming/gcloud")).unwrap(); + std::fs::create_dir_all(home.join("Downloads")).unwrap(); + std::fs::write(home.join("AppData/Roaming/gcloud/credentials.db"), "DUMMY_CREDENTIALS").unwrap(); + std::fs::write(home.join("saved.html"), "

saved

").unwrap(); + std::fs::write(home.join("notes.txt"), "DUMMY_NOTES").unwrap(); + std::fs::write(dir.path().join("top.html"), "

top

").unwrap(); + std::fs::write(home.join("Downloads/page.html"), "

page

").unwrap(); + std::fs::write(home.join("Downloads/app.js"), "export {}").unwrap(); + + let server = WebServer { home: Some(std::fs::canonicalize(&home).unwrap()), ..WebServer::new() }; + let saved = server.url_for(None, &home.join("saved.html")).await.unwrap(); + assert!(get(&saved, None).await.ends_with("

saved

")); + for other in ["/AppData/Roaming/gcloud/credentials.db", "/notes.txt", "/", "/Downloads/app.js"] { + let page = get(&saved.replace("/saved.html", other), None).await; + assert!(page.starts_with("HTTP/1.1 404"), "{other}: {page}"); + } + let top = server.url_for(None, &dir.path().join("top.html")).await.unwrap(); + assert!(get(&top, None).await.ends_with("

top

")); + let page = get(&top.replace("/top.html", "/home/AppData/Roaming/gcloud/credentials.db"), None).await; + assert!(page.starts_with("HTTP/1.1 404"), "{page}"); + // Below the home folder a page still loads what lies beside it. + let page = server.url_for(None, &home.join("Downloads/page.html")).await.unwrap(); + assert!(get(&page.replace("/page.html", "/app.js"), None).await.ends_with("export {}")); + } + + /// A file outside the project whose own name starts with a dot is not refused: it is + /// served alone. + #[tokio::test] + async fn a_dot_file_outside_the_project_is_served_alone() { + let dir = tempfile::tempdir().unwrap(); + let folder = dir.path().join("folder"); + std::fs::create_dir_all(&folder).unwrap(); + std::fs::write(folder.join(".draft.html"), "

draft

").unwrap(); + std::fs::write(folder.join("app.js"), "export {}").unwrap(); + + let server = server(); + let url = server.url_for(None, &folder.join(".draft.html")).await.unwrap(); + assert!(get(&url, None).await.ends_with("

draft

")); + assert!(get(&url.replace("/.draft.html", "/app.js"), None).await.starts_with("HTTP/1.1 404")); + } + + #[tokio::test] + async fn a_page_outside_the_project_loads_what_lies_beside_it() { + let dir = tempfile::tempdir().unwrap(); + let site = dir.path().join("site"); + std::fs::create_dir_all(site.join("assets")).unwrap(); + std::fs::write(site.join("page.html"), "

hi

").unwrap(); + std::fs::write(site.join("app.js"), "export {}").unwrap(); + std::fs::write(site.join("assets/style.css"), "h1 {}").unwrap(); + + let server = server(); + let url = server.url_for(None, &site.join("page.html")).await.unwrap(); + assert!(get(&url, None).await.ends_with("

hi

")); + assert!(get(&url.replace("/page.html", "/app.js"), None).await.ends_with("export {}")); + assert!(get(&url.replace("/page.html", "/assets/style.css"), None).await.ends_with("h1 {}")); + } + + /// A project is served whole: build output often lies in a dot folder (`.vitepress/dist`). + /// The same folder asked for as a file outside another project keeps its hidden files. + #[tokio::test] + async fn the_project_serves_its_dot_folders() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("docs"); + std::fs::create_dir_all(root.join(".vitepress/dist")).unwrap(); + std::fs::write(root.join(".vitepress/dist/index.html"), "

built

").unwrap(); + std::fs::write(root.join("readme.html"), "

readme

").unwrap(); + + let server = server(); + let outside = server.url_for(None, &root.join("readme.html")).await.unwrap(); + let built = server.url_for(Some(&root), &root.join(".vitepress/dist/index.html")).await.unwrap(); + let page = get(&built, None).await; + assert!(page.starts_with("HTTP/1.1 200") && page.ends_with("

built

"), "{page}"); + let hidden = get(&outside.replace("/readme.html", "/.vitepress/dist/index.html"), None).await; + assert!(hidden.starts_with("HTTP/1.1 404"), "{hidden}"); + } + + /// Security audit lead 7: a project that is the home folder, or holds it, is not served + /// whole. Its pages are served like pages outside a project. + #[tokio::test] + async fn a_project_that_holds_the_home_folder_is_not_served_whole() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path().join("home"); + std::fs::create_dir_all(home.join(".ssh")).unwrap(); + std::fs::create_dir_all(home.join("Downloads")).unwrap(); + std::fs::write(home.join(".ssh/id_ed25519"), "DUMMY_KEY").unwrap(); + std::fs::write(home.join("saved.html"), "

saved

").unwrap(); + std::fs::write(home.join("notes.txt"), "DUMMY_NOTES").unwrap(); + std::fs::write(home.join("Downloads/page.html"), "

page

").unwrap(); + std::fs::write(home.join("Downloads/app.js"), "export {}").unwrap(); + + let server = WebServer { home: Some(std::fs::canonicalize(&home).unwrap()), ..WebServer::new() }; + for project in [home.as_path(), dir.path()] { + let page = server.url_for(Some(project), &home.join("Downloads/page.html")).await.unwrap(); + assert!(get(&page, None).await.ends_with("

page

")); + assert!(get(&page.replace("/page.html", "/app.js"), None).await.ends_with("export {}")); + // Where the key lies when the project is served whole. + let key = get(&page.replace("/Downloads/page.html", "/.ssh/id_ed25519"), None).await; + assert!(key.starts_with("HTTP/1.1 404"), "{}: {key}", project.display()); + let saved = server.url_for(Some(project), &home.join("saved.html")).await.unwrap(); + assert!(get(&saved, None).await.ends_with("

saved

")); + for other in ["/.ssh/id_ed25519", "/notes.txt"] { + let got = get(&saved.replace("/saved.html", other), None).await; + assert!(got.starts_with("HTTP/1.1 404"), "{} {other}: {got}", project.display()); + } + } + } + + /// With no home folder to compare with, a file outside the project is served alone. + #[tokio::test] + async fn a_page_outside_the_project_is_served_alone_when_the_home_folder_is_unknown() { + let dir = tempfile::tempdir().unwrap(); + let (folder, project) = (dir.path().join("folder"), dir.path().join("project")); + std::fs::create_dir_all(&folder).unwrap(); + std::fs::create_dir_all(&project).unwrap(); + for at in [&folder, &project] { + std::fs::write(at.join("page.html"), "

page

").unwrap(); + std::fs::write(at.join("app.js"), "export {}").unwrap(); + } + + let server = WebServer { home: None, ..WebServer::new() }; + let page = server.url_for(None, &folder.join("page.html")).await.unwrap(); + assert!(get(&page, None).await.ends_with("

page

")); + assert!(get(&page.replace("/page.html", "/app.js"), None).await.starts_with("HTTP/1.1 404")); + // A project is served whole all the same. + let page = server.url_for(Some(&project), &project.join("page.html")).await.unwrap(); + assert!(get(&page.replace("/page.html", "/app.js"), None).await.ends_with("export {}")); + } + + /// A file served alone is named by its resolved path, which a missing file has not. + #[tokio::test] + async fn a_missing_file_that_would_be_served_alone_has_no_url() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path().join("home"); + std::fs::create_dir_all(&home).unwrap(); + let server = WebServer { home: Some(std::fs::canonicalize(&home).unwrap()), ..WebServer::new() }; + assert!(server.url_for(None, &home.join("missing.html")).await.is_err()); + } } diff --git a/crates/ide-workspace/src/file_refs.rs b/crates/ide-workspace/src/file_refs.rs index d2484aa61..d927db28a 100644 --- a/crates/ide-workspace/src/file_refs.rs +++ b/crates/ide-workspace/src/file_refs.rs @@ -93,11 +93,21 @@ pub fn base_dirs(path: &str, from: &Path, root: Option<&Path>, resource_roots: & dirs } +/// Whether a path written in `from` may be looked up: joining a share's path replaces the +/// directory it is joined to, and looking it up signs in to that share's server. +fn reachable(path: &Path, from: &Path) -> bool { + ide_vfs::content_path_allowed(path, &[from]) +} + /// Paths `value` may point at, most likely first. pub fn candidates(value: &str, from: &Path, root: Option<&Path>, resource_roots: &[PathBuf]) -> Vec { let path = path_part(value); let rel = path.trim_start_matches('/'); - base_dirs(&path, from, root, resource_roots).iter().map(|d| normalize(&d.join(rel))).collect() + base_dirs(&path, from, root, resource_roots) + .iter() + .map(|d| normalize(&d.join(rel))) + .filter(|p| reachable(p, from)) + .collect() } /// The existing file or directory `value` names, if any. @@ -132,7 +142,11 @@ pub fn complete(prefix: &str, from: &Path, root: Option<&Path>, resource_roots: if base == Path::new("/") && !out.is_empty() { continue; } - let Ok(entries) = std::fs::read_dir(normalize(&base.join(rel))) else { continue }; + let dir = normalize(&base.join(rel)); + if !reachable(&dir, from) { + continue; + } + let Ok(entries) = std::fs::read_dir(dir) else { continue }; for e in entries.flatten() { let name = e.file_name().to_string_lossy().into_owned(); if (name.starts_with('.') && !show_hidden) || out.iter().any(|c| c.name == name) { @@ -209,4 +223,45 @@ mod tests { ); assert_eq!(names("/img/"), [("logo.svg".into(), false)]); } + + /// A path written in a page must not make the host open a share on another server: on + /// Windows that signs in to it with the user's credentials. The inspection of unresolved + /// references looks every path up as soon as the page opens. + #[cfg(windows)] + #[test] + fn references_reach_no_share_but_the_one_the_page_is_on() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("web/img")).unwrap(); + for f in ["web/index.html", "web/img/a.png"] { + std::fs::write(root.join(f), "").unwrap(); + } + // The same folder reached over SMB through the loopback admin share. + let s = root.to_str().unwrap(); + let share = PathBuf::from(format!(r"\\127.0.0.1\{}${}", &s[..1], &s[2..])); + let image = share.join(r"web\img\a.png"); + assert!(image.exists(), "control: {} is not reachable over SMB, fixture not live", image.display()); + let unc = image.to_str().unwrap(); + + let from = root.join("web/index.html"); + // The last one is the NT spelling `\??\UNC\…` (a bare `?` would start the query). + for value in [unc.to_owned(), unc.replace('\\', "%5C"), format!(r"/\%3F%3F\UNC{}", &unc[1..])] { + assert_eq!(resolve(&value, &from, Some(root), &[]), None, "{value}"); + } + let typed = format!("{}/", share.join("web").display()); + assert_eq!(complete(&typed, &from, Some(root), &[]), [], "{typed}"); + + // A page that is on the share names its own files. + let from = share.join(r"web\index.html"); + assert_eq!(resolve("img/a.png", &from, Some(&share), &[]), Some(image.clone())); + assert_eq!(complete("img/", &from, Some(&share), &[]), [PathCompletion { name: "a.png".into(), dir: false }]); + // The same page as the webview names it: the canonical path, with `/`. + let from = PathBuf::from(ide_core::ipc_path::to_ipc(&dunce::canonicalize(&from).unwrap())); + assert!(from.starts_with("//?/UNC/127.0.0.1"), "{}", from.display()); + assert_eq!(resolve("img/a.png", &from, None, &[]), Some(from.parent().unwrap().join("img/a.png"))); + assert_eq!(resolve(unc, &from, None, &[]), Some(image.clone())); + let other = format!(r"\\localhost\{}", &unc[r"\\127.0.0.1\".len()..]); + assert!(Path::new(&other).exists(), "control: {other} is not reachable over SMB"); + assert_eq!(resolve(&other, &from, None, &[]), None); + } } diff --git a/crates/ide-workspace/src/workspace.rs b/crates/ide-workspace/src/workspace.rs index 1fc9f429f..9815eccab 100644 --- a/crates/ide-workspace/src/workspace.rs +++ b/crates/ide-workspace/src/workspace.rs @@ -128,10 +128,24 @@ fn valid_name(name: &str) -> IdeResult { Ok(name.to_owned()) } +/// A workspace file can come with a cloned repository, and looking a folder up on a network +/// share signs in to that share's server: only local drives and the share `from` is on are +/// looked up. +fn reachable(path: &Path, from: &Path) -> IdeResult<()> { + if ide_vfs::content_path_allowed(path, &[from]) { + return Ok(()); + } + Err(IdeError::invalid(format!( + "Project folder is not on a local drive or the workspace's share: {}", + path.display() + ))) +} + fn normalize_projects(root: &Path, projects: Vec) -> IdeResult> { let mut result = Vec::new(); for path in projects { let path = if path.is_absolute() { path } else { root.join(path) }; + reachable(&path, root)?; let path = match dunce::canonicalize(&path) { Ok(path) if path.is_dir() => path, Err(e) if e.kind() == std::io::ErrorKind::NotFound => path, @@ -141,6 +155,9 @@ fn normalize_projects(root: &Path, projects: Vec) -> IdeResult { + const asked: string[] = []; + await page.route(/^https:\/\/([a-z\d-]+\.)*example\.com\//, (route) => { + asked.push(new URL(route.request().url()).pathname); + return route.fulfill({ contentType: "image/gif", body: Buffer.from(PIXEL.split(",")[1]!, "base64") }); + }); + const at = (name: string) => `https://images.example.com/${name}`; + const fence = (...lines: string[]) => ["```mermaid", ...lines, "```"].join("\n"); + const diagrams = [ + fence( + `%%{init: {"themeCSS": ".nodeLabel{background-image:url(${at("theme-css")})} svg{background-image:url(${at("theme-css-root")})}"}}%%`, + "flowchart TD", + " A[One]-->B[Two]", + ), + fence( + `%%{init: {"fontFamily": "x;background-image:url(${at("font-family")})"}}%%`, + "flowchart TD", + " A[One]-->B[Two]", + ), + fence( + "---", + "config:", + ` themeCSS: ".nodeLabel{background-image:url(${at("front-matter")})}"`, + "---", + "flowchart TD", + " A[One]-->B[Two]", + ), + ]; + const reply = await openReply(page, `Diagrams:\n\n${diagrams.join("\n\n")}`); + await expect(reply.locator("[data-mermaid-state='ok'] .mermaid-output svg")).toHaveCount(diagrams.length); + await page.evaluate( + (src) => + new Promise((done) => { + const img = new Image(); + img.onload = img.onerror = () => done(); + img.src = src; + }), + "https://example.com/after", + ); + expect(asked).toEqual(["/after"]); +}); diff --git a/plugins/README.md b/plugins/README.md index 2021bb80b..dce5b21b1 100644 --- a/plugins/README.md +++ b/plugins/README.md @@ -345,7 +345,7 @@ machine. Users can revoke optional capabilities in Settings › Plugins. | `ide.commands` | high | Run any IDE action or other plugins' commands as if you chose them, including running programs, committing and pushing. API 1.4; declared own commands are exempt. | | `database.sources` | medium | API 1.4: manage only the plugin's own project database sources, write their secure credentials and request connection; trusted projects only. | | `project.read` | low | `Project::root`, `read_file`, `read_bytes`, `find_files`, `symbols`, `search_text`, `host::projects`, `publish_diagnostics`, `problems`, editor opening and decorations | -| `project.write` | medium | `Project::write_file` — creates and replaces text files inside the open projects | +| `project.write` | high | `Project::write_file` — creates and replaces text files inside the open projects, the settings and build scripts a trusted project runs included | | `fs.read:` | high | `host::fs::read_file`, `read_bytes`, `stat`, `read_dir` — files outside the projects, under the grant's directory | | `fs.write:` | high | `host::fs::write_file`, `write_bytes` — files under that path; doesn't imply reading | | `editor.read` | low | `TextEditor::document`, `selections`, `host::open_editors` | @@ -388,7 +388,9 @@ keep the file's encoding and BOM (a new file is UTF-8), are capped at 16 MiB, and are refused while the editor holds unsaved changes for the same file. The path is resolved as a read's is, so a link inside a granted directory cannot be used to write outside it. `project.write` does the same inside the open -projects, creating missing directories. Both are new in API 1.3 +projects, creating missing directories. It never writes git's own files: +nothing inside a `.git` folder, nor the `.git` file of a linked worktree or +submodule, whichever name or link leads there. Both are new in API 1.3 (`engines.openidea = "^1.3"`). `host::fs::plugin_dir()` (API 1.3) returns a private directory belonging only to @@ -1006,7 +1008,8 @@ Then `register_plugin!(MyPlugin, providers)` and implement `inline_completion`, `code_actions`, `definition`, `line_markers`. Each gets the document (path, language, version, text). Results join the language servers' (completion lists, ⌥⏎ quick fixes, ⌘B, gutter icons; a code action -carries edits for any files, applied as one change). A completion item's +carries edits for files in the project, applied as one change; an edit +anywhere else, or to git's own files, disables it). A completion item's `kind` (`CompletionKind` in the SDK, a string in the WIT) picks its icon; it can be a `snippet` (`$1`, `${1:name}`, `$0`), match on `filter_text` instead of its label, and bring `additional_edits` elsewhere in the document (an @@ -1023,7 +1026,8 @@ deletes files implements `file_actions::FileActions` too covers both): a `FileAction` carries `operations` (`FileOperation::Create { path, text, overwrite }`, `Rename`, `Delete`) and `edits`, applied as one workspace edit, so a failure changes nothing and one undo reverses it. A path -outside the project root disables the action; `open` shows a file once the +outside the project root, or one of git's own files (a `.git` folder's, or the +`.git` file of a linked worktree), disables the action; `open` shows a file once the action is applied. Like a code action, it can leave `operations` and `edits` empty and set `data`: `resolve_file_action` computes them when the user picks it, with the same prompts and editor window as `resolve_code_action`. File actions are new in API 1.3. Problems (`publish_diagnostics`) can name diff --git a/src/editor/lsp/links.test.ts b/src/editor/lsp/links.test.ts index 801d3b616..3e60fabf4 100644 --- a/src/editor/lsp/links.test.ts +++ b/src/editor/lsp/links.test.ts @@ -71,8 +71,25 @@ describe("links and fragments", () => { const text = Text.of(['']); const range = { start: { line: 0, col: 9 }, end: { line: 0, col: 19 } }; const link = { range, path: "/p/b.html", fragment: "top", url: null, tooltip: null }; - expect(linkReference(text, [link], 12)).toMatchObject({ kind: "file", path: "/p/b.html", fragment: "top" }); - expect(linkReference(text, [link], 2)).toBeNull(); + expect(linkReference(text, [link], 12, "/p/a.html")).toMatchObject({ + kind: "file", + path: "/p/b.html", + fragment: "top", + }); + expect(linkReference(text, [link], 2, "/p/a.html")).toBeNull(); + }); + + // On Windows, opening `//server/share` signs in to that server with the user's credentials. + it("follows no server link to a share the file is not on", () => { + const text = Text.of(['']); + const range = { start: { line: 0, col: 9 }, end: { line: 0, col: 35 } }; + const link = { range, path: "//evil/share/b.html", fragment: null, url: null, tooltip: null }; + expect(linkReference(text, [link], 12, "C:/p/a.html")).toBeNull(); + expect(linkReference(text, [link], 12, "//nas/work/a.html")).toBeNull(); + expect(linkReference(text, [{ ...link, path: "//nas/work/b.html" }], 12, "//NAS/Work/a.html")).toMatchObject({ + kind: "file", + path: "//nas/work/b.html", + }); }); it("locates line, element id and Markdown heading fragments", () => { diff --git a/src/editor/lsp/links.ts b/src/editor/lsp/links.ts index 4deb96950..53d7fd17b 100644 --- a/src/editor/lsp/links.ts +++ b/src/editor/lsp/links.ts @@ -10,6 +10,7 @@ import type { Text } from "@codemirror/state"; import type { EditorView } from "@codemirror/view"; import { whenSynced, type EditorDocContext } from "@/editor/api"; import { call, commands, isTauri, type DocumentLink } from "@/platform/ipc"; +import { onOtherShare } from "@/platform/paths"; import { openExternalUrl } from "@/platform/openUrl"; import { executeAction } from "@/platform/actions"; import { editorService } from "@/workbench/contributions"; @@ -53,10 +54,17 @@ export function knownLinks(view: EditorView): DocumentLink[] | undefined { return fetchedLinks.get(view.state.doc); } -export function linkReference(text: Text, links: readonly DocumentLink[], offset: number): Reference | null { +/** The server's link around `offset` in the document at `path`; none to a network share that document is not on. */ +export function linkReference( + text: Text, + links: readonly DocumentLink[], + offset: number, + path: string, +): Reference | null { for (const l of links) { const { from, to } = rangeToOffsets(text, l.range); if (offset < from || offset > to) continue; + if (l.path && onOtherShare(l.path, path)) return null; if (l.path) return { kind: "file", from, to, path: l.path, fragment: l.fragment }; if (l.url) return { kind: "url", from, to, url: l.url }; } @@ -195,7 +203,7 @@ export async function followReference(ref: Reference): Promise { * named by a quoted path. Null when there is none. */ export async function referenceAt(view: EditorView, doc: EditorDocContext, offset: number): Promise { - const link = linkReference(view.state.doc, await documentLinks(view, doc), offset); + const link = linkReference(view.state.doc, await documentLinks(view, doc), offset, doc.path); const ref = textReference(view.state.doc, offset); if (link?.kind !== "file") { if (link) return link; diff --git a/src/editor/lsp/markdown.test.ts b/src/editor/lsp/markdown.test.ts index c3b5e2f51..9ac5ae3be 100644 --- a/src/editor/lsp/markdown.test.ts +++ b/src/editor/lsp/markdown.test.ts @@ -1,4 +1,7 @@ import { describe, expect, it, vi } from "vitest"; +import type { ProjectInfo } from "@/platform/ipc"; +import { useProject } from "@/platform/project"; +import { sourceLink } from "./markdown"; // A fresh module per test: the parser is loaded lazily and cached module-wide. async function freshMarkdown() { @@ -56,3 +59,25 @@ describe("markdown rendering", () => { expect(md.markupToHtml({ kind: "plaintext", value: "" })).toBe('

<b>

'); }); }); + +// On Windows, opening `//server/share` signs in to that server with the user's credentials. +describe("sourceLink", () => { + const project = (root: string | null) => + useProject.setState({ project: root === null ? null : ({ root } as ProjectInfo) }); + + it("reads the file and position a documentation link names", () => { + project("C:/repo"); + expect(sourceLink("file:///C:/repo/src/a.py#L12,5")).toEqual({ path: "/C:/repo/src/a.py", line: 11, col: 4 }); + expect(sourceLink("https://example.com/a.py")).toBeNull(); + }); + + it("names no share the project is not on", () => { + for (const root of ["C:/repo", "//nas/work/app", null]) { + project(root); + expect(sourceLink("file:////evil/share/a.py#L3"), String(root)).toBeNull(); + expect(sourceLink("file:///%5C%5Cevil/share/a.py"), String(root)).toBeNull(); + } + project("//nas/work/app"); + expect(sourceLink("file:////NAS/work/lib/a.py#L3")).toEqual({ path: "//NAS/work/lib/a.py", line: 2, col: 0 }); + }); +}); diff --git a/src/editor/lsp/markdown.ts b/src/editor/lsp/markdown.ts index a1e85ee46..acbe3b30c 100644 --- a/src/editor/lsp/markdown.ts +++ b/src/editor/lsp/markdown.ts @@ -13,6 +13,8 @@ import { classHighlighter, highlightTree } from "@lezer/highlight"; import type { Markup } from "@/bindings"; import { maybeEditorService } from "@/workbench/contributions"; import { isTauri } from "@/platform/ipc"; +import { onOtherShare } from "@/platform/paths"; +import { useProject } from "@/platform/project"; import { isVirtualUri } from "@/platform/virtualDocs"; import { selectableText } from "@/ui/selectableText"; @@ -203,9 +205,12 @@ export function sourceLink(href: string): { path: string; line: number; col: num } catch { return null; } + const path = decodeURIComponent(url.pathname); + // Documentation is text someone else wrote. + if (onOtherShare(path, useProject.getState().project?.root ?? "")) return null; const m = /^#?L?(\d+)(?:[,:](\d+))?/.exec(url.hash.slice(1)); return { - path: decodeURIComponent(url.pathname), + path, line: m ? Math.max(0, Number(m[1]) - 1) : 0, col: m?.[2] ? Math.max(0, Number(m[2]) - 1) : 0, }; diff --git a/src/editor/lsp/navigation.ts b/src/editor/lsp/navigation.ts index 146469188..3c09256c3 100644 --- a/src/editor/lsp/navigation.ts +++ b/src/editor/lsp/navigation.ts @@ -740,7 +740,8 @@ function linkAt(view: EditorView, x: number, y: number): { from: number; to: num const pos = view.posAtCoords({ x, y }, false); if (pos == null) return null; const links = knownLinks(view); - const ref = (links && linkReference(view.state.doc, links, pos)) ?? textReference(view.state.doc, pos); + const ref = + (links && linkReference(view.state.doc, links, pos, docOf(view)?.path ?? "")) ?? textReference(view.state.doc, pos); if (ref) return { from: ref.from, to: ref.to, link: true }; const word = navigationWord(view, pos); return word && word.from < word.to ? { from: word.from, to: word.to, link: false } : null; diff --git a/src/features/agents/Markdown.test.tsx b/src/features/agents/Markdown.test.tsx index b095e8232..2a7ad522f 100644 --- a/src/features/agents/Markdown.test.tsx +++ b/src/features/agents/Markdown.test.tsx @@ -155,13 +155,24 @@ it("loads raw HTML images without allowing alternate sources or forged paths", a expect(container.querySelector("source, [srcset], [onerror]")).toBeNull(); }); -it.each([PIXEL, "https://example.com/code.png", "http://localhost:9000/code.png"])( - "keeps an inline or web image: %s", +it("keeps an inline image: a data URL loads without a request", () => { + render(); + expect(screen.getByRole("img")).toHaveAttribute("src", PIXEL); + expect(screen.getByRole("img")).toHaveAttribute("referrerpolicy", "no-referrer"); + expect(readImage).not.toHaveBeenCalled(); +}); + +it.each(["https://example.com/code.png", "http://localhost:9000/code.png"])( + "loads a web image in a reply only on a click: %s", (src) => { - render(); - expect(screen.getByRole("img")).toHaveAttribute("src", src); - expect(screen.getByRole("img")).toHaveAttribute("referrerpolicy", "no-referrer"); + const { container } = render(); + const img = container.querySelector("img")!; + // Not fetched on render: a button stands in, the image is hidden with no src. + expect(img).not.toHaveAttribute("src"); expect(readImage).not.toHaveBeenCalled(); + fireEvent.click(screen.getByRole("button", { name: `Load image from ${new URL(src).host}` })); + expect(img).toHaveAttribute("src", src); + expect(img).toHaveAttribute("referrerpolicy", "no-referrer"); }, ); @@ -213,10 +224,23 @@ it.each(["javascript:alert(1)", "data:text/html,test", "/tmp/%ZZ.png", "/tmp/cod ); it("reports network or decode failures without leaving a broken source", () => { - render(); - fireEvent.error(screen.getByRole("img")); - expect(screen.getByRole("img")).not.toHaveAttribute("src"); - expect(screen.getByRole("img")).toHaveAttribute("alt", "Unable to load image: https://example.com/gone.png"); + const { container } = render(); + fireEvent.click(screen.getByRole("button", { name: "Load image from example.com" })); + const img = container.querySelector("img")!; + fireEvent.error(img); + expect(img).not.toHaveAttribute("src"); + expect(img).toHaveAttribute("alt", "Unable to load image: https://example.com/gone.png"); +}); + +it("drops a reply attribute that names another host, so nothing fetches on render", () => { + const { container } = render( +

x

'} + cwd="/repo" + />, + ); + expect(container.querySelector("[poster]")).toBeNull(); + expect(container.querySelector("[background]")).toBeNull(); }); describe("mermaid diagrams in a reply", () => { diff --git a/src/features/agents/Markdown.tsx b/src/features/agents/Markdown.tsx index d601dc49d..b10760868 100644 --- a/src/features/agents/Markdown.tsx +++ b/src/features/agents/Markdown.tsx @@ -19,7 +19,7 @@ import { editorService } from "@/workbench/contributions"; import { hasMermaid, renderMermaidIn } from "@/features/viewers/markdown/mermaid"; import { subscribeTheme, themeKey } from "@/platform/themeWatch"; import { fileHref, pathLink, type PathLink } from "./conversation"; -import { loadImages } from "./markdownImages"; +import { loadImages, type MessageImages } from "./markdownImages"; import styles from "./Session.module.css"; const escapeHtml = (s: string) => @@ -42,9 +42,20 @@ const RENDERED_MAX = 500; let purifier: ReturnType | null = null; +/** Whether the webview would read `value` as an address on another host: it drops blanks and takes `\` for `/`. */ +const namesHost = (value: string) => + /^(https?:|[\\/]{2})/i.test([...value].filter((c) => c.charCodeAt(0) > 32).join("")); + function purify(): ReturnType { if (purifier) return purifier; const p = DOMPurify(window); + // A reply reaches another host only when the user asks: a link they click, an image `loadImages` holds. + // `poster`, `background` and the `src` of other tags would fetch as soon as the reply renders. + p.addHook("uponSanitizeAttribute", (node, data) => { + if (data.attrName === "alt" || data.attrName === "title") return; + if (data.attrName === "href" && node.tagName === "A") return; + if (namesHost(data.attrValue)) data.keepAttr = false; + }); const sources = new WeakMap(); // Preserve image-only file URLs without relaxing URI sanitizing for links or other tags. p.addHook("beforeSanitizeAttributes", (node) => { @@ -176,7 +187,7 @@ export function openPathLink(link: PathLink) { } /** Highlighting, Copy buttons and file links over freshly rendered `root`. */ -function enhance(root: HTMLElement, cwd: string, images: Map>): () => void { +function enhance(root: HTMLElement, cwd: string, images: MessageImages): () => void { let alive = true; const disposeImages = loadImages(root, cwd, images); const copies: HTMLButtonElement[] = []; @@ -243,7 +254,7 @@ function paintBlocks( painted: PaintedBlock[], sources: string[], cwd: string, - images: Map>, + images: MessageImages, ) { let same = 0; while (same < painted.length && same < sources.length && painted[same]!.source === sources[same]) same++; @@ -271,7 +282,7 @@ export function Markdown({ onDiagramDrawn?: () => void; }) { const ref = useRef(null); - const images = useRef(new Map>()); + const images = useRef({ reads: new Map(), asked: new Set() }); /** The blocks on screen, so a streamed chunk repaints only the blocks it changed. */ const painted = useRef([]); const paintedCwd = useRef(cwd); diff --git a/src/features/agents/Session.module.css b/src/features/agents/Session.module.css index acbb58929..1ad51522e 100644 --- a/src/features/agents/Session.module.css +++ b/src/features/agents/Session.module.css @@ -540,6 +540,18 @@ .markdown :global(img) { max-width: 100%; } +.loadImage { + padding: 2px 8px; + border: 1px solid var(--border); + border-radius: var(--radius-small); + background: var(--bg-elev); + color: var(--fg-secondary); + font: inherit; + cursor: pointer; +} +.loadImage:hover { + background: var(--bg-hover); +} .markdown :global(li:has(> input[type="checkbox"])) { margin-left: -20px; } diff --git a/src/features/agents/conversation.test.ts b/src/features/agents/conversation.test.ts index 36d0c8547..aba72412c 100644 --- a/src/features/agents/conversation.test.ts +++ b/src/features/agents/conversation.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { findAll, findRanges, formatDuration, pathLink, quoteText, withQuote } from "./conversation"; +import { fileHref, findAll, findRanges, formatDuration, pathLink, quoteText, withQuote } from "./conversation"; describe("quoting", () => { it("quotes each line and leaves a line to type on", () => { @@ -37,6 +37,33 @@ describe("pathLink", () => { }); }); +describe("fileHref", () => { + const cwd = "C:/repo"; + it("resolves a link against the task folder", () => { + expect(fileHref("docs/my%20notes.md:3", cwd)).toEqual({ path: "C:/repo/docs/my notes.md", line: 2 }); + expect(fileHref("/abs/x.py#L7", cwd)).toEqual({ path: "/abs/x.py", line: 6 }); + expect(fileHref("https://x.dev/a.md", cwd)).toBeNull(); + expect(fileHref("#top", cwd)).toBeNull(); + }); + // On Windows, opening `//server/share` signs in to that server with the user's credentials. + it("names no network share, however the link spells it", () => { + for (const href of [ + "//evil/share/x.md", + "%2F%2Fevil/share/x.md", + "/%2Fevil/share/x.md", + "/\\evil/share/x.md", + "/%5Cevil/share/x.md", + "%5C%5Cevil%5Cshare%5Cx.md", + ]) { + expect(fileHref(href, cwd), href).toBeNull(); + } + expect(fileHref("%5Cevil/share/x.md", ""), "no task folder").toBeNull(); + }); + it("follows links in a task that is on a share", () => { + expect(fileHref("docs/a.md", "//nas/work/app")).toEqual({ path: "//nas/work/app/docs/a.md" }); + }); +}); + describe("formatDuration", () => { it("scales the unit", () => { expect(formatDuration(850)).toBe("850ms"); diff --git a/src/features/agents/conversation.ts b/src/features/agents/conversation.ts index 253098e63..7415973d7 100644 --- a/src/features/agents/conversation.ts +++ b/src/features/agents/conversation.ts @@ -1,5 +1,7 @@ /** Reading an agent conversation: quoting, file links, turn times and finding text. */ +import { opensOtherShare } from "@/platform/paths"; + /** `text` as a Markdown quote for the composer, followed by an empty line to type in. */ export function quoteText(text: string): string { const lines = text.replace(/\s+$/, "").split("\n"); @@ -24,7 +26,7 @@ const PATH = /^(?:\.\/)?([\w.@-]+(?:\/[\w.@-]+)*\.[A-Za-z][A-Za-z0-9]*)(?::(\d+) /** Explicit Markdown links can also name files with spaces or non-ASCII names. */ export function fileHref(href: string, cwd: string): PathLink | null { - if (!href || href.startsWith("#") || href.startsWith("//") || /^[a-z][a-z\d+.-]*:/i.test(href)) return null; + if (!href || href.startsWith("#") || /^[a-z][a-z\d+.-]*:/i.test(href)) return null; let decoded: string; try { decoded = decodeURIComponent(href); @@ -34,6 +36,7 @@ export function fileHref(href: string, cwd: string): PathLink | null { const match = /^(.*?)(?::(\d+)(?::(\d+))?|#L(\d+))?$/.exec(decoded); if (!match?.[1] || [...decoded].some((char) => char.charCodeAt(0) < 32)) return null; const path = match[1].startsWith("/") ? match[1] : `${cwd.replace(/\/$/, "")}/${match[1]}`; + if (opensOtherShare(path, match[1], cwd)) return null; const link: PathLink = { path }; const line = match[2] ?? match[4]; if (line) link.line = Math.max(0, Number(line) - 1); diff --git a/src/features/agents/markdownImages.ts b/src/features/agents/markdownImages.ts index 658dd1acb..301e28e35 100644 --- a/src/features/agents/markdownImages.ts +++ b/src/features/agents/markdownImages.ts @@ -1,6 +1,7 @@ import { call, commands } from "@/platform/ipc"; import { ipcPath } from "@/platform/paths"; import { tx } from "@/platform/i18n"; +import styles from "./Session.module.css"; const hasControl = (text: string) => [...text].some((c) => c.charCodeAt(0) < 32 || c.charCodeAt(0) === 127); @@ -37,9 +38,12 @@ export function imageTarget(source: string, cwd: string): ImageTarget | null { } } -/** Cache belongs to one message: streaming reuses reads, later replies can show a new QR at the same path. */ -export function loadImages(root: HTMLElement, cwd: string, cache: Map>): () => void { +/** What one message has loaded: streaming reuses it, later replies read a new QR at the same path and ask again. */ +export type MessageImages = { reads: Map>; asked: Set }; + +export function loadImages(root: HTMLElement, cwd: string, { reads, asked }: MessageImages): () => void { let alive = true; + const held: HTMLButtonElement[] = []; const images = [...root.querySelectorAll("img")]; for (const img of images) { const source = img.dataset.imageSrc ?? ""; @@ -55,13 +59,34 @@ export function loadImages(root: HTMLElement, cwd: string, cache: Map { + asked.add(url); + load.remove(); + img.hidden = false; + img.src = url; + }; + img.hidden = true; + img.before(load); + held.push(load); + } } else { - let pending = cache.get(target.path); + let pending = reads.get(target.path); if (!pending) { pending = call(commands.fsReadImage(target.path)); - cache.set(target.path, pending); - void pending.catch(() => cache.delete(target.path)); + reads.set(target.path, pending); + void pending.catch(() => reads.delete(target.path)); } void pending.then((url) => { if (alive) img.src = url; @@ -71,5 +96,6 @@ export function loadImages(root: HTMLElement, cwd: string, cache: Map { alive = false; for (const img of images) img.onerror = null; + for (const load of held) load.remove(); }; } diff --git a/src/features/viewers/markdown/mermaid.ts b/src/features/viewers/markdown/mermaid.ts index a2398c3a3..cd11cf364 100644 --- a/src/features/viewers/markdown/mermaid.ts +++ b/src/features/viewers/markdown/mermaid.ts @@ -180,6 +180,8 @@ function configure(mermaid: Mermaid, key: string) { mermaid.initialize({ startOnLoad: false, securityLevel: "strict", + // These go into the diagram's stylesheet as written: a diagram setting them could fetch from any host. + secure: ["themeCSS", "fontFamily", "altFontFamily"], suppressErrorRendering: true, theme: "base", // Each diagram type defaults to "neo": the start state is hard-coded diff --git a/src/features/viewers/markdown/render.test.ts b/src/features/viewers/markdown/render.test.ts index 41931713d..fba98248f 100644 --- a/src/features/viewers/markdown/render.test.ts +++ b/src/features/viewers/markdown/render.test.ts @@ -91,6 +91,31 @@ describe("markdown preview rendering", () => { expect(root.textContent).not.toContain("outline"); }); + it("lets only a link name another host", () => { + const at = (name: string) => `https://example.com/${name}`; + const root = document.createElement("div"); + root.innerHTML = renderMarkdown( + [ + `[A link](${at("link")})`, + ``, + ``, + ``, + ``, + `
cell
`, + '', + String.raw``, + ].join("\n\n"), + ); + const named = [...root.querySelectorAll("*")].flatMap((el) => + [...el.attributes] + .filter((attr) => attr.value.includes("example.com")) + .map((attr) => `${el.localName}[${attr.name}]`), + ); + expect(named).toEqual(["a[href]"]); + expect([...root.querySelectorAll("img")].map((img) => img.dataset.src)).toEqual(["a.png", "a.png"]); + expect(root.querySelector("td")?.textContent).toBe("cell"); + }); + it("keeps the raw HTML READMEs rely on", () => { const root = document.createElement("div"); root.innerHTML = renderMarkdown( @@ -204,6 +229,24 @@ describe("markdown preview rendering", () => { expect(joinPath("/repo", "../../a.md")).toBe("/a.md"); }); + // On Windows, opening `//server/share` signs in to that server with the user's credentials. + it("resolves no link to a network share, however the file spells it", () => { + const file = "C:/repo/docs/guide.md"; + for (const href of [ + "/\\evil/share/x.png", + "/%5Cevil/share/x.png", + "//evil/share/x.png", + "/%2Fevil/share/x.png", + "file:///%5Cevil/share/x.png", + "file:////evil/share/x.png", + "%5C%5Cevil%5Cshare%5Cx.png", + ]) { + expect(resolveLink(file, href), href).toBeNull(); + } + expect(resolveLink("/guide.md", "../%5Cevil/share/x.png"), "file at the root").toBeNull(); + expect(resolveLink(file, "/docs/a.png")).toEqual({ kind: "file", path: "/docs/a.png" }); + }); + it("interpolates the preview offset between blocks", () => { const blocks = [ { line: 0, top: 0 }, diff --git a/src/features/viewers/markdown/render.ts b/src/features/viewers/markdown/render.ts index 4fac3cbd0..f670d08ea 100644 --- a/src/features/viewers/markdown/render.ts +++ b/src/features/viewers/markdown/render.ts @@ -8,6 +8,7 @@ */ import { Marked, type Token, type TokensList } from "marked"; import DOMPurify from "dompurify"; +import { opensOtherShare } from "@/platform/paths"; const escapeHtml = (s: string) => s.replace(/&/g, "&").replace(//g, ">").replace(/"/g, """); @@ -26,9 +27,21 @@ const marked = new Marked({ // Own instance: hooks must not leak into the hover / documentation renderer. let purifier: ReturnType | null = null; +/** Whether the webview would read `value` as an address on another host: it drops blanks and takes `\` for `/`. */ +const namesHost = (value: string) => + /^(https?:|[\\/]{2})/i.test([...value].filter((c) => c.charCodeAt(0) > 32).join("")); + function purify(): ReturnType { if (purifier) return purifier; const p = DOMPurify(window); + // The preview loads nothing from other hosts. Only a link, opened on a click, names one; + // an image keeps its source for the preview to refuse. + p.addHook("uponSanitizeAttribute", (node, data) => { + if (data.attrName === "alt" || data.attrName === "title") return; + if (data.attrName === "href" && node.tagName === "A") return; + if (data.attrName === "src" && node.tagName === "IMG") return; + if (namesHost(data.attrValue)) data.keepAttr = false; + }); p.addHook("afterSanitizeAttributes", (node) => { if (node.tagName === "IMG") { const src = node.getAttribute("src"); @@ -165,8 +178,9 @@ export function renderMarkdown(text: string): string { return purify().sanitize(html, { USE_PROFILES: { html: true }, ADD_ATTR: ["data-src", "data-line", "data-front-matter"], - FORBID_TAGS: ["style", "form"], - FORBID_ATTR: ["style"], + // `srcset` and `` would pick an image past the one the preview loads. + FORBID_TAGS: ["style", "form", "source"], + FORBID_ATTR: ["style", "srcset"], }); } @@ -203,6 +217,7 @@ export function resolveLink(file: string, href: string): LinkTarget | null { const fragment = hash >= 0 ? decodeURIComponent(rest.slice(hash + 1)) : undefined; const pathPart = decodeURIComponent((hash >= 0 ? rest.slice(0, hash) : rest).replace(/\?.*$/, "")); const path = pathPart ? joinPath(dirOf(file), pathPart) : file; + if (pathPart && opensOtherShare(path, pathPart, file)) return null; const lineMatch = fragment ? /^L(\d+)/.exec(fragment) : null; if (lineMatch) return { kind: "file", path, line: Math.max(0, Number(lineMatch[1]) - 1) }; return { kind: "file", path, ...(fragment ? { anchor: fragment } : {}) }; diff --git a/src/platform/locales/text/en.json b/src/platform/locales/text/en.json index 5a2731aa2..aff1b0526 100644 --- a/src/platform/locales/text/en.json +++ b/src/platform/locales/text/en.json @@ -349,6 +349,7 @@ "Listening for Xdebug on port {port}": "Listening for Xdebug on port {port}", "Port {port} is already in use: another IDE or debug session may be listening on it. Stop it or choose another port.": "Port {port} is already in use: another IDE or debug session may be listening on it. Stop it or choose another port.", "Unable to load image: {path}": "Unable to load image: {path}", + "Load image from {host}": "Load image from {host}", "Updates": "Updates", "Update source": "Update source", "Current version": "Current version", diff --git a/src/platform/locales/text/zh.json b/src/platform/locales/text/zh.json index 760f0ae57..0e51f2558 100644 --- a/src/platform/locales/text/zh.json +++ b/src/platform/locales/text/zh.json @@ -6820,6 +6820,7 @@ "Stage and Retry": "暂存并重试", "Show Files": "显示文件", "Unable to load image: {path}": "无法加载图片:{path}", + "Load image from {host}": "加载来自 {host} 的图片", "Updates": "更新", "Update source": "更新源", "Current version": "当前版本", diff --git a/src/platform/paths.test.ts b/src/platform/paths.test.ts index e7b455a17..282d1665b 100644 --- a/src/platform/paths.test.ts +++ b/src/platform/paths.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { ipcPath } from "./paths"; +import { ipcPath, onOtherShare, opensOtherShare } from "./paths"; const raw = String.raw; const APPDATA = raw`C:\Users\me\AppData\Roaming\ai.l0ng.openidea`; @@ -22,3 +22,57 @@ describe("ipcPath", () => { expect(ipcPath(raw`/home/me/a\b.txt`)).toBe(raw`/home/me/a\b.txt`); }); }); + +describe("onOtherShare", () => { + it("is true for a share the base is not on", () => { + for (const base of ["C:/repo/a.md", "", "//nas/work/a.md", "//evil/other/a.md", "//evil"]) { + expect(onOtherShare("//evil/share/a.png", base), base).toBe(true); + expect(onOtherShare(raw`\\evil\share\a.png`, base), base).toBe(true); + } + // A server alone, and spellings with no server to compare. + for (const path of ["//evil", "//evil/", raw`/\\evil/share/a.png`]) { + expect(onOtherShare(path, "//evil/share/a.md"), path).toBe(true); + } + }); + + // A project on a share gets its paths from the host as `//?/UNC/server/share/…`. + it("reads the server and share of a device path", () => { + const base = "//?/UNC/nas/work/app/a.md"; + expect(onOtherShare("//?/UNC/evil/share/a.png", base)).toBe(true); + expect(onOtherShare("//?/UNC/nas/private/a.png", base)).toBe(true); + expect(onOtherShare("//evil/share/a.png", base)).toBe(true); + expect(onOtherShare("//?/UNC", base)).toBe(true); + expect(onOtherShare("//?/C:/a.png", "//?/C:/a.md")).toBe(true); + expect(onOtherShare("//?/unc/NAS/Work/lib/a.png", base)).toBe(false); + expect(onOtherShare("//nas/work/lib/a.png", base)).toBe(false); + expect(onOtherShare("//?/UNC/nas/work/lib/a.png", "//nas/work/app/a.md")).toBe(false); + }); + + it("is false on the base's own share and off any share", () => { + expect(onOtherShare("//NAS/Work/lib/a.png", "//nas/work/app/a.md")).toBe(false); + expect(onOtherShare(raw`\\nas\work\lib\a.png`, "//nas/work/app/a.md")).toBe(false); + expect(onOtherShare("C:/repo/a.png", "//nas/work/app/a.md")).toBe(false); + expect(onOtherShare("/abs/a.png", "C:/repo/a.md")).toBe(false); + }); +}); + +describe("opensOtherShare", () => { + it("is true for a share the text names itself, in any mix of slashes", () => { + for (const written of ["//evil/share/a.png", raw`\\evil\share\a.png`, raw`/\evil/share/a.png`, raw`\/evil/a.png`]) { + expect(opensOtherShare(written, written, "C:/repo"), written).toBe(true); + expect(opensOtherShare(written, written, "//nas/work"), written).toBe(true); + } + }); + + it("is true for a share that only appears once the path is resolved", () => { + expect(opensOtherShare(raw`/\evil/share/a.png`, raw`\evil/share/a.png`, "")).toBe(true); + expect(opensOtherShare(raw`/\evil/share/a.png`, raw`../\evil/share/a.png`, "/guide.md")).toBe(true); + expect(opensOtherShare("//evil/share/a.png", "a.png", "//nas/work")).toBe(true); + }); + + it("is false on a local drive and on the share the text is on", () => { + expect(opensOtherShare("C:/repo/docs/a.png", "docs/a.png", "C:/repo")).toBe(false); + expect(opensOtherShare("/abs/a.png", "/abs/a.png", "C:/repo")).toBe(false); + expect(opensOtherShare("//nas/work/docs/a.png", "docs/a.png", "//nas/work")).toBe(false); + }); +}); diff --git a/src/platform/paths.ts b/src/platform/paths.ts index 0e3cc6d69..5a3ed2082 100644 --- a/src/platform/paths.ts +++ b/src/platform/paths.ts @@ -21,6 +21,39 @@ export function ipcPath(p: string): string { return DRIVE.test(path) || NETWORK.test(path) ? path.replace(/\\/g, "/") : path; } +const SHARE = /^[\\/]{2}/; +const SHARE_NAME = /^[\\/]{2}(?:\?[\\/]UNC[\\/])?([^\\/]+)[\\/]([^\\/]+)/i; + +/** + * `server/share` of a path on a network share, in lower case; null for any other path. The + * host hands out a share's paths as `//?/UNC/server/share/…`, the dialogs as `//server/share/…`. + */ +function shareOf(path: string): string | null { + const m = SHARE_NAME.exec(path); + // Any other device path (`//?/C:/…`, `//./…`) names no server to compare. + return m && m[1] !== "?" && m[1] !== "." ? `${m[1]}/${m[2]}`.toLowerCase() : null; +} + +/** + * Whether `path` is on a network share that `base` is not on. Windows reads `//server/share`, + * `\\server\share` and mixes of the two as a share, and opening one signs in to its server + * with the user's credentials: a path that text someone else wrote leads to is followed to no + * share but the one that text is on. + */ +export function onOtherShare(path: string, base: string): boolean { + if (!SHARE.test(path)) return false; + const share = shareOf(path); + return share === null || share !== shareOf(base); +} + +/** + * `onOtherShare` for a path resolved against `base`, the folder or file of the text naming + * it: also true when that text spells a share itself (`written`, decoded). + */ +export function opensOtherShare(path: string, written: string, base: string): boolean { + return SHARE.test(written) || onOtherShare(path, base); +} + /** True when `path` equals `ancestor` or lies below it. */ export function isSameOrInside(path: string, ancestor: string): boolean { path = ipcPath(path);