From 68657b8215b1d662b2f0099967aecc26597c064e Mon Sep 17 00:00:00 2001 From: Matias Palma Date: Mon, 20 Apr 2026 22:41:29 -0400 Subject: [PATCH 1/2] fix: allow-list git_url before install_extension spawns git clone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install_extension forwarded the caller-supplied git_url straight to git clone. Any scheme went through — file://, Windows UNC, ssh://, git://, and ext:: transport helpers — which meant a compromised catalog entry could trigger arbitrary code at install time via git helpers, symlink tricks, or credential-embedding URLs. Added validate_git_clone_url before the clone call. It enforces: - Strict https:// scheme (case-sensitive). - Host in a small allowlist (github.com, gitlab.com, bitbucket.org). Every legitimate catalog entry today already satisfies this, and resolveGitRepoUrl on the frontend produces exactly this shape. - Path is exactly `/` or `/.git`, with ASCII-safe segments that cannot start with `-` (no flag injection via --upload-pack and friends). - No credentials (`@`), no transport-helper syntax (`::`), no whitespace or control characters. As belt-and-suspenders, the clone invocation now also passes `-c protocol.ext.allow=never -c protocol.file.allow=never -c protocol.git.allow=never` so git itself refuses those transports even if the URL check is ever bypassed. The allow-listed https host is unaffected. Added 12 unit tests covering the accept/reject matrix. --- apps/desktop/src-tauri/src/extensions.rs | 165 +++++++++++++++++++++++ 1 file changed, 165 insertions(+) diff --git a/apps/desktop/src-tauri/src/extensions.rs b/apps/desktop/src-tauri/src/extensions.rs index 9d1714ad..ea7552d7 100644 --- a/apps/desktop/src-tauri/src/extensions.rs +++ b/apps/desktop/src-tauri/src/extensions.rs @@ -266,8 +266,92 @@ fn get_extensions_dir(app: &AppHandle) -> Result { Ok(ext_dir) } +/// Hosts we allow `git clone` to target from the extension marketplace. Every +/// legitimate entry today points at one of these, and the constraint blocks +/// the bulk of the attack surface: `file://`, Windows UNC, `ssh://`, +/// `git://`, and `ext::` transports can't reach this point because the host +/// won't match, and typo-squatted domains have nowhere to land. +const ALLOWED_GIT_HOSTS: &[&str] = &["github.com", "gitlab.com", "bitbucket.org"]; + +/// Enforces the URL shape that `git clone` will accept: strict `https://`, +/// allow-listed host, exactly `owner/repo[.git]`, ASCII-safe segments, no +/// credentials, no transport-helper syntax. Everything coming out of +/// `resolveGitRepoUrl` on the frontend already looks like this, so the +/// check is free for legitimate catalog entries and hard-stops a compromised +/// registry. +fn validate_git_clone_url(url: &str) -> Result<(), String> { + // Reject control characters and whitespace up front — these should never + // appear in a real clone URL and catching them here keeps downstream + // argv/process parsers honest. + if url.chars().any(|c| c.is_control() || c == ' ') { + return Err("git_url must not contain control characters or whitespace".to_string()); + } + + // Block the `ext::` transport-helper syntax that lets git run arbitrary + // helper binaries. The https:// strip below would catch this too, but + // the explicit message helps when debugging a compromised catalog. + if url.contains("::") { + return Err("git_url must not contain transport-helper syntax (`::`)".to_string()); + } + + let after_scheme = url + .strip_prefix("https://") + .ok_or_else(|| format!("git_url must begin with https:// (got `{}`)", url))?; + + // `user@host/...` shapes shift the host parser and let an attacker embed + // credentials or, worse, alternate hosts. No legitimate marketplace + // entry uses them. + if after_scheme.contains('@') { + return Err("git_url must not contain `@` (credentials/alternate host)".to_string()); + } + + let (host, path) = after_scheme + .split_once('/') + .ok_or_else(|| format!("git_url is missing a repository path: `{}`", url))?; + + if !ALLOWED_GIT_HOSTS.contains(&host) { + return Err(format!( + "git_url host `{}` is not in the allowlist ({})", + host, + ALLOWED_GIT_HOSTS.join(", ") + )); + } + + let segments: Vec<&str> = path.split('/').filter(|s| !s.is_empty()).collect(); + if segments.len() != 2 { + return Err(format!( + "git_url path must be `/` or `/.git`; got `{}`", + path + )); + } + + for seg in &segments { + if seg.starts_with('-') { + return Err(format!( + "git_url path segment `{}` cannot start with `-`", + seg + )); + } + if !seg + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') + { + return Err(format!( + "git_url path segment `{}` contains disallowed characters", + seg + )); + } + } + + Ok(()) +} + #[tauri::command] pub async fn install_extension(app: AppHandle, id: String, git_url: String) -> Result<(), String> { + // Validate before we even build the target directory, so a bad URL never + // reaches `git clone`. + validate_git_clone_url(&git_url)?; + let ext_dir = get_extensions_dir(&app)?; let target_dir = ext_dir.join(&id); @@ -275,7 +359,18 @@ pub async fn install_extension(app: AppHandle, id: String, git_url: String) -> R return Err("Extension is already installed".into()); } + // Belt-and-suspenders: even though `validate_git_clone_url` already + // enforces an https:// allow-listed host, turn off the protocols that are + // most commonly abused for arbitrary command execution during clone. If + // anything ever gets past the URL check, git itself will still refuse + // `ext::`, `file://`, and raw `git://`. let output = Command::new("git") + .arg("-c") + .arg("protocol.ext.allow=never") + .arg("-c") + .arg("protocol.file.allow=never") + .arg("-c") + .arg("protocol.git.allow=never") .arg("clone") .arg("--depth") .arg("1") @@ -407,3 +502,73 @@ pub async fn read_extension_script(app: AppHandle, id: String) -> Result Date: Mon, 20 Apr 2026 23:05:08 -0400 Subject: [PATCH 2/2] fix: validate extension id to block path-traversal via catalog id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on #168: `id` was forwarded straight into `ext_dir.join(&id)` across install/uninstall/update/is_active/toggle/read_script. A compromised catalog entry could therefore set `id = "..\..\..\Windows\foo"` and cause `git clone` (or `remove_dir_all`, or a file read) to operate **outside** the app-data extensions sandbox. Added `validate_extension_id` that enforces: - Non-empty, not `.`, not `..`. - No path separators (`/`, `\`) and no `..` substring. - No leading `-` (so the id can never be reparsed as a flag elsewhere). - Characters restricted to ASCII alphanumeric + `-`, `_`, `.` — the safe marketplace-slug charset that every current catalog entry already satisfies. Applied to every Tauri command that takes an `id: String`: `install_extension`, `uninstall_extension`, `update_extension`, `is_extension_active`, `toggle_extension_state`, `read_extension_script`. The string-only commands (`is_active`, `toggle_state`) don't join `id` to a path today, but validating them keeps a malicious catalog entry from poisoning `disabled_extensions.json` with invalid entries that later code may still path-join. Added 5 unit tests covering accept/reject cases including the explicit traversal shapes (`../evil`, `..\\..\\Windows`, `foo..bar`). --- apps/desktop/src-tauri/src/extensions.rs | 87 +++++++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src-tauri/src/extensions.rs b/apps/desktop/src-tauri/src/extensions.rs index ea7552d7..e8236cdb 100644 --- a/apps/desktop/src-tauri/src/extensions.rs +++ b/apps/desktop/src-tauri/src/extensions.rs @@ -346,10 +346,49 @@ fn validate_git_clone_url(url: &str) -> Result<(), String> { Ok(()) } +/// Reject extension ids that would escape `ext_dir` on join, contain +/// path separators, parent-directory components, or anything other than +/// the safe marketplace-style slug characters. Required because a +/// compromised catalog could supply `id = "../../../Windows/foo"` and +/// `ext_dir.join(id)` on Windows happily resolves that outside the +/// extensions directory — `git clone` would then write to the escaped +/// location instead of the sandboxed app-data subfolder. +fn validate_extension_id(id: &str) -> Result<(), String> { + if id.is_empty() { + return Err("extension id is empty".to_string()); + } + if id == "." || id == ".." { + return Err(format!( + "extension id `{}` is not a valid directory name", + id + )); + } + if id.contains('/') || id.contains('\\') || id.contains("..") { + return Err(format!( + "extension id `{}` cannot contain path separators or `..`", + id + )); + } + if id.starts_with('-') { + return Err(format!("extension id `{}` cannot start with `-`", id)); + } + if !id + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') + { + return Err(format!( + "extension id `{}` contains disallowed characters (allowed: ASCII alphanumeric, `-`, `_`, `.`)", + id + )); + } + Ok(()) +} + #[tauri::command] pub async fn install_extension(app: AppHandle, id: String, git_url: String) -> Result<(), String> { - // Validate before we even build the target directory, so a bad URL never - // reaches `git clone`. + // Validate id and git_url before we even build the target directory so a + // bad input never reaches `git clone` or the filesystem. + validate_extension_id(&id)?; validate_git_clone_url(&git_url)?; let ext_dir = get_extensions_dir(&app)?; @@ -390,6 +429,7 @@ pub async fn install_extension(app: AppHandle, id: String, git_url: String) -> R #[tauri::command] pub async fn uninstall_extension(app: AppHandle, id: String) -> Result<(), String> { + validate_extension_id(&id)?; let ext_dir = get_extensions_dir(&app)?; let target_dir = ext_dir.join(&id); @@ -402,6 +442,7 @@ pub async fn uninstall_extension(app: AppHandle, id: String) -> Result<(), Strin #[tauri::command] pub async fn update_extension(app: AppHandle, id: String) -> Result<(), String> { + validate_extension_id(&id)?; let ext_dir = get_extensions_dir(&app)?; let target_dir = ext_dir.join(&id); @@ -448,6 +489,7 @@ pub async fn get_installed_extensions(app: AppHandle) -> Result, Str #[tauri::command] pub async fn is_extension_active(app: AppHandle, id: String) -> Result { + validate_extension_id(&id)?; let app_data = app.path().app_data_dir().map_err(|e| e.to_string())?; let disabled_file = app_data.join("disabled_extensions.json"); @@ -467,6 +509,7 @@ pub async fn toggle_extension_state( id: String, is_active: bool, ) -> Result<(), String> { + validate_extension_id(&id)?; let app_data = app.path().app_data_dir().map_err(|e| e.to_string())?; let disabled_file = app_data.join("disabled_extensions.json"); @@ -491,6 +534,7 @@ pub async fn toggle_extension_state( #[tauri::command] pub async fn read_extension_script(app: AppHandle, id: String) -> Result { + validate_extension_id(&id)?; let ext_dir = get_extensions_dir(&app)?; let target_dir = ext_dir.join(&id); let index_file = target_dir.join("index.js"); @@ -572,3 +616,42 @@ mod git_url_validation_tests { assert!(validate_git_clone_url("https://github.com/a/b\n.git").is_err()); } } + +#[cfg(test)] +mod extension_id_validation_tests { + use super::validate_extension_id; + + #[test] + fn accepts_plain_slug() { + assert!(validate_extension_id("trixty.example-addon").is_ok()); + assert!(validate_extension_id("my_ext_01").is_ok()); + } + + #[test] + fn rejects_empty_or_dot_segments() { + assert!(validate_extension_id("").is_err()); + assert!(validate_extension_id(".").is_err()); + assert!(validate_extension_id("..").is_err()); + } + + #[test] + fn rejects_path_separators_and_parent_references() { + assert!(validate_extension_id("a/b").is_err()); + assert!(validate_extension_id("a\\b").is_err()); + assert!(validate_extension_id("../evil").is_err()); + assert!(validate_extension_id("..\\..\\Windows").is_err()); + assert!(validate_extension_id("foo..bar").is_err()); + } + + #[test] + fn rejects_leading_dash() { + assert!(validate_extension_id("-flagish").is_err()); + } + + #[test] + fn rejects_non_ascii_and_special_chars() { + assert!(validate_extension_id("ext ension").is_err()); + assert!(validate_extension_id("ext%20").is_err()); + assert!(validate_extension_id("exté").is_err()); + } +}