fix(security): escape consent client name; UUID-validate space-config paths

- Escape the attacker-controlled DCR/CIMD `client_name` before interpolating
  it into the /oauth/authorize consent HTML — closes reflected XSS (HIGH-2).
- `get_space_config_path` now parses `space_id` as a UUID, closing path
  traversal / Windows absolute-path replacement across every space-config
  command; `open_space_config_file` opens via `tauri-plugin-opener` instead of
  `cmd /C start`, removing the OS-command-injection path (HIGH-3).
- Clone the ClientMetadataService handle out of `GatewayState` and drop the
  read guard before the CIMD HTTP fetch, so a slow fetch no longer holds the
  write-preferring state lock across all MCP requests.

Signed-off-by: Mohammod Al Amin Ashik <maa.ashik00@gmail.com>

Author: its-mash

apps/desktop/src-tauri/src/commands/space.rs

@@ -69,7 +69,7 @@ pub async fn create_space(
         .map_err(|e| e.to_string())?;
 
     // Create default config file for the space (spaces_dir already exists via AppState::new)
-    let config_path = state.space_config_path(&space.id.to_string());
+    let config_path = state.space_config_path(&space.id.to_string())?;
 
     // Create default config file if it doesn't exist
     if !config_path.exists() {
@@ -143,9 +143,7 @@ pub async fn open_space_config_file(
     space_id: String,
     state: State<'_, AppState>,
 ) -> Result<(), String> {
-    use std::process::Command;
-
-    let config_path = state.space_config_path(&space_id);
+    let config_path = state.space_config_path(&space_id)?;
 
     if !config_path.exists() {
         return Err(format!(
@@ -154,32 +152,11 @@ pub async fn open_space_config_file(
         ));
     }
 
-    // Open in default editor based on platform
-    #[cfg(target_os = "windows")]
-    {
-        Command::new("cmd")
-            .args(["/C", "start", "", config_path.to_str().unwrap()])
-            .spawn()
-            .map_err(|e| format!("Failed to open file: {}", e))?;
-    }
-
-    #[cfg(target_os = "macos")]
-    {
-        Command::new("open")
-            .arg(&config_path)
-            .spawn()
-            .map_err(|e| format!("Failed to open file: {}", e))?;
-    }
-
-    #[cfg(target_os = "linux")]
-    {
-        Command::new("xdg-open")
-            .arg(&config_path)
-            .spawn()
-            .map_err(|e| format!("Failed to open file: {}", e))?;
-    }
-
-    Ok(())
+    // Open with the OS default handler via the opener plugin — never via a
+    // shell. The previous `cmd /C start <path>` form let cmd.exe interpret
+    // metacharacters in the (then-unvalidated) path: OS command injection.
+    tauri_plugin_opener::open_path(&config_path, None::<&str>)
+        .map_err(|e| format!("Failed to open file: {}", e))
 }
 
 /// Read space configuration file
@@ -188,7 +165,7 @@ pub async fn read_space_config(
     space_id: String,
     state: State<'_, AppState>,
 ) -> Result<String, String> {
-    let config_path = state.space_config_path(&space_id);
+    let config_path = state.space_config_path(&space_id)?;
 
     // Create default config if it doesn't exist (for spaces created before this feature)
     if !config_path.exists() {
@@ -210,7 +187,7 @@ pub async fn save_space_config(
     content: String,
     state: State<'_, AppState>,
 ) -> Result<(), String> {
-    let config_path = state.space_config_path(&space_id);
+    let config_path = state.space_config_path(&space_id)?;
 
     // Validate JSON before saving
     serde_json::from_str::<serde_json::Value>(&content)
@@ -226,7 +203,7 @@ pub async fn remove_server_from_config(
     server_id: String,
     state: State<'_, AppState>,
 ) -> Result<bool, String> {
-    let config_path = state.space_config_path(&space_id);
+    let config_path = state.space_config_path(&space_id)?;
 
     // If config file doesn't exist, nothing to remove
     if !config_path.exists() {

apps/desktop/src-tauri/src/state/mod.rs

@@ -193,8 +193,12 @@ impl AppState {
         &self.spaces_dir
     }
 
-    /// Get the path to a specific space's config file
-    pub fn space_config_path(&self, space_id: &str) -> PathBuf {
+    /// Get the path to a specific space's config file.
+    ///
+    /// Fails when `space_id` is not a valid UUID — the id arrives over IPC,
+    /// so this is the path-traversal guard for every space-config command.
+    pub fn space_config_path(&self, space_id: &str) -> Result<PathBuf, String> {
         mcpmux_core::get_space_config_path(&self.spaces_dir, space_id)
+            .map_err(|e| format!("Invalid space id '{space_id}': {e}"))
     }
 }

crates/mcpmux-core/src/lib.rs

@@ -36,7 +36,14 @@ pub use event_bus::{
 
 use std::path::{Path, PathBuf};
 
-/// Get the path to a space's configuration file (relative to a base spaces directory)
-pub fn get_space_config_path(spaces_dir: &Path, space_id: &str) -> PathBuf {
-    spaces_dir.join(format!("{}.json", space_id))
+/// Get the path to a space's configuration file (relative to a base spaces directory).
+///
+/// `space_id` must parse as a UUID — IPC callers pass attacker-influenceable
+/// strings, and joining them raw would allow path traversal (`../..`) or, on
+/// Windows, full path replacement (`Path::join` with an absolute path
+/// discards the base). The canonical hyphenated form of the *parsed* UUID is
+/// used as the filename, never the raw input.
+pub fn get_space_config_path(spaces_dir: &Path, space_id: &str) -> Result<PathBuf, uuid::Error> {
+    let id = uuid::Uuid::parse_str(space_id)?;
+    Ok(spaces_dir.join(format!("{}.json", id)))
 }

crates/mcpmux-gateway/src/server/handlers.rs

@@ -173,66 +173,71 @@ pub async fn oauth_authorize(
         );
     }
 
-    // Resolve and validate client (CIMD or traditional)
-    {
+    // Resolve and validate client (CIMD or traditional).
+    //
+    // IMPORTANT: clone the service handle out and DROP the state lock before
+    // `resolve_client()` — CIMD client ids resolve via an outbound HTTP fetch
+    // (10 s timeout), and `oauth_middleware` takes this same write-preferring
+    // RwLock on every MCP request, so a read guard held across the fetch plus
+    // one queued writer would stall all MCP traffic.
+    let client_metadata_service = {
         let gateway_state = state.read().await;
+        gateway_state.client_metadata_service_arc()
+    };
+    let Some(client_metadata_service) = client_metadata_service else {
+        error!("[OAuth] ClientMetadataService not available");
+        return oauth_error_redirect(
+            &params.redirect_uri,
+            "server_error",
+            "Service not available",
+            params.state.as_deref(),
+        );
+    };
 
-        let client_metadata_service = match gateway_state.client_metadata_service() {
-            Some(s) => s,
-            None => {
-                error!("[OAuth] ClientMetadataService not available");
-                return oauth_error_redirect(
-                    &params.redirect_uri,
-                    "server_error",
-                    "Service not available",
-                    params.state.as_deref(),
-                );
-            }
-        };
-
-        // Resolve client (handles CIMD URL or traditional client_id)
-        let client = match client_metadata_service
-            .resolve_client(&params.client_id)
-            .await
-        {
-            Ok(Some(c)) => c,
-            Ok(None) => {
-                warn!("[OAuth] Unknown client_id: {}", params.client_id);
-                return oauth_error_redirect(
-                    &params.redirect_uri,
-                    "invalid_client",
-                    "Client not registered",
-                    params.state.as_deref(),
+    // Resolve client (handles CIMD URL or traditional client_id). The same
+    // resolution also yields the consent page's display name — resolve once.
+    let display_name = match client_metadata_service
+        .resolve_client(&params.client_id)
+        .await
+    {
+        Ok(Some(client)) => {
+            // Validate redirect_uri against resolved client.
+            // Per RFC 8252 §7.3, loopback redirect URIs are matched ignoring the port,
+            // since native public clients use an ephemeral OS-assigned port at request
+            // time that may differ from the one captured at DCR.
+            if !is_redirect_uri_allowed(&client.redirect_uris, &params.redirect_uri) {
+                warn!(
+                    "[OAuth] Invalid redirect_uri for client: {} (expected one of: {:?})",
+                    params.redirect_uri, client.redirect_uris
                 );
-            }
-            Err(e) => {
-                error!("[OAuth] Client resolution failed: {}", e);
                 return oauth_error_redirect(
                     &params.redirect_uri,
-                    "server_error",
-                    "Client resolution error",
+                    "invalid_redirect_uri",
+                    "Redirect URI not registered for this client",
                     params.state.as_deref(),
                 );
             }
-        };
-
-        // Validate redirect_uri against resolved client.
-        // Per RFC 8252 §7.3, loopback redirect URIs are matched ignoring the port,
-        // since native public clients use an ephemeral OS-assigned port at request
-        // time that may differ from the one captured at DCR.
-        if !is_redirect_uri_allowed(&client.redirect_uris, &params.redirect_uri) {
-            warn!(
-                "[OAuth] Invalid redirect_uri for client: {} (expected one of: {:?})",
-                params.redirect_uri, client.redirect_uris
+            client.client_name
+        }
+        Ok(None) => {
+            warn!("[OAuth] Unknown client_id: {}", params.client_id);
+            return oauth_error_redirect(
+                &params.redirect_uri,
+                "invalid_client",
+                "Client not registered",
+                params.state.as_deref(),
             );
+        }
+        Err(e) => {
+            error!("[OAuth] Client resolution failed: {}", e);
             return oauth_error_redirect(
                 &params.redirect_uri,
-                "invalid_redirect_uri",
-                "Redirect URI not registered for this client",
+                "server_error",
+                "Client resolution error",
                 params.state.as_deref(),
             );
         }
-    }
+    };
 
     // PKCE is required for public clients
     if params.code_challenge.is_none() {
@@ -262,19 +267,6 @@ pub async fn oauth_authorize(
         params.client_id
     );
 
-    // Get client display name from metadata service for new clients
-    let display_name = {
-        let gateway_state = state.read().await;
-        if let Some(service) = gateway_state.client_metadata_service() {
-            match service.resolve_client(&params.client_id).await {
-                Ok(Some(client)) => client.client_name,
-                _ => "Unknown Application".to_string(),
-            }
-        } else {
-            "Unknown Application".to_string()
-        }
-    };
-
     // Store pending authorization request with expiration (5 minutes)
     let request_id = uuid::Uuid::new_v4().to_string();
     let expires_at = std::time::SystemTime::now()
@@ -323,6 +315,12 @@ pub async fn oauth_authorize(
 
     let app_name = branding::DISPLAY_NAME;
 
+    // HTML-escape the client-supplied display name before interpolating it
+    // into the consent page — DCR/CIMD `client_name` is attacker-controlled
+    // (reflected XSS otherwise). The raw name stays on the pending
+    // authorization for the desktop UI, which renders it as text via React.
+    let display_name_html = html_escape_text(&display_name);
+
     // HTML page that triggers the deep link
     // The page shows a brief message while the app opens
     // Industry standard: Don't auto-close, let user close after approval
@@ -435,7 +433,7 @@ pub async fn oauth_authorize(
         </p>
 
         <div class="client-info">
-            <div class="client-name">{display_name}</div>
+            <div class="client-name">{display_name_html}</div>
             <div class="client-id">wants to connect</div>
         </div>
 
@@ -473,6 +471,24 @@ pub async fn oauth_authorize(
     axum::response::Html(html).into_response()
 }
 
+/// Minimal HTML entity escaping for untrusted text interpolated into
+/// gateway-served HTML. Covers every character that can break out of a
+/// text node or a double-quoted attribute value.
+fn html_escape_text(s: &str) -> String {
+    let mut out = String::with_capacity(s.len());
+    for c in s.chars() {
+        match c {
+            '&' => out.push_str("&amp;"),
+            '<' => out.push_str("&lt;"),
+            '>' => out.push_str("&gt;"),
+            '"' => out.push_str("&quot;"),
+            '\'' => out.push_str("&#39;"),
+            _ => out.push(c),
+        }
+    }
+    out
+}
+
 /// Helper to create OAuth error redirect
 fn oauth_error_redirect(
     redirect_uri: &str,

crates/mcpmux-gateway/src/server/state.rs

@@ -126,6 +126,18 @@ impl GatewayState {
         self.client_metadata_service.as_ref().map(|s| s.as_ref())
     }
 
+    /// Clone the client metadata service handle out of the state.
+    ///
+    /// Use this (and drop the state guard) before calling
+    /// `resolve_client()`: CIMD client ids resolve via an outbound HTTP
+    /// fetch (10 s timeout), and `GatewayState`'s write-preferring RwLock
+    /// is taken by `oauth_middleware` on every MCP request — holding a
+    /// read guard across the fetch can stall all MCP traffic behind one
+    /// queued writer.
+    pub fn client_metadata_service_arc(&self) -> Option<Arc<ClientMetadataService>> {
+        self.client_metadata_service.clone()
+    }
+
     /// Check if database is connected
     pub fn has_database(&self) -> bool {
         self.db.is_some()