Skip to content

Commit 66583b8

Browse files
committed
fix: harden extensions module with safety and robustness improvements
- Replace per-call Regex::new().unwrap() with once_cell::sync::Lazy statics in types.rs for NAME_RE and VERSION_RE patterns - Replace Mutex::lock().expect() with unwrap_or_else(|e| e.into_inner()) in wasm/host.rs for all global registry locks (consistent with existing pattern) - Fix unwrap_or(Ok(...)) to unwrap_or_else(|_| ...) in wasm/mod.rs - Add manifest validation on extension load in state.rs - Add zip bomb protection with entry count and total size limits in utils.rs - Skip symlinks in copy_dir_recursive to prevent symlink attacks - Add path traversal prevention (reject '..' components) in validate_workspace_path - Add URI validation in host_get_document_text_by_uri - Implement graceful shutdown with 5s timeout for Node.js extension host process
1 parent ca7f8f6 commit 66583b8

File tree

6 files changed

+89
-23
lines changed

6 files changed

+89
-23
lines changed

src-tauri/src/extensions/node_host/process.rs

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,29 @@ impl NodeHostProcess {
122122

123123
pub async fn stop(&self) -> Result<(), String> {
124124
let mut child = self.child.lock().await;
125-
child
126-
.kill()
127-
.await
128-
.map_err(|e| format!("Failed to kill extension host: {}", e))?;
129-
info!("Node.js extension host stopped");
125+
126+
// Try graceful shutdown first by closing stdin
127+
drop(self.stdin.lock().await);
128+
129+
// Wait up to 5 seconds for the process to exit gracefully
130+
let graceful = tokio::time::timeout(
131+
std::time::Duration::from_secs(5),
132+
child.wait(),
133+
)
134+
.await;
135+
136+
match graceful {
137+
Ok(Ok(_)) => {
138+
info!("Node.js extension host stopped gracefully");
139+
}
140+
_ => {
141+
warn!("Extension host did not exit gracefully, forcing kill");
142+
child
143+
.kill()
144+
.await
145+
.map_err(|e| format!("Failed to kill extension host: {}", e))?;
146+
}
147+
}
130148
Ok(())
131149
}
132150

src-tauri/src/extensions/state.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use std::path::PathBuf;
1010
use std::sync::Arc;
1111
use tracing::{info, warn};
1212

13-
use super::types::{Extension, ExtensionManifest, ExtensionSource};
13+
use super::types::{validate_manifest, Extension, ExtensionManifest, ExtensionSource};
1414
use super::utils::{copy_dir_recursive, extensions_directory_path};
1515
#[cfg(feature = "wasm-extensions")]
1616
use super::wasm::WasmRuntime;
@@ -102,6 +102,15 @@ impl ExtensionsManager {
102102
let manifest: ExtensionManifest = serde_json::from_str(&manifest_content)
103103
.map_err(|e| format!("Failed to parse extension.json: {}", e))?;
104104

105+
if let Err(errors) = validate_manifest(&manifest) {
106+
let messages: Vec<String> = errors.iter().map(|e| e.message.clone()).collect();
107+
warn!(
108+
"Manifest validation warnings for '{}': {}",
109+
manifest.name,
110+
messages.join("; ")
111+
);
112+
}
113+
105114
let enabled = self
106115
.enabled_extensions
107116
.get(&manifest.name)

src-tauri/src/extensions/types.rs

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,22 @@
33
//! This module contains all the data structures used to represent extensions,
44
//! their manifests, and their contributions.
55
6+
use once_cell::sync::Lazy;
67
use regex::Regex;
78
use serde::{Deserialize, Serialize};
89
use std::collections::HashMap;
910
use std::path::PathBuf;
1011

12+
static NAME_RE: Lazy<Regex> = Lazy::new(|| {
13+
#[allow(clippy::unwrap_used)]
14+
Regex::new(r"^[a-zA-Z0-9._-]+$").unwrap()
15+
});
16+
17+
static VERSION_RE: Lazy<Regex> = Lazy::new(|| {
18+
#[allow(clippy::unwrap_used)]
19+
Regex::new(r"^\d+\.\d+\.\d+$").unwrap()
20+
});
21+
1122
/// Extension manifest schema - defines the structure of extension.json
1223
#[derive(Debug, Clone, Serialize, Deserialize)]
1324
pub struct ExtensionManifest {
@@ -511,24 +522,20 @@ pub struct ManifestValidationError {
511522
pub fn validate_manifest(manifest: &ExtensionManifest) -> Result<(), Vec<ManifestValidationError>> {
512523
let mut errors: Vec<ManifestValidationError> = Vec::new();
513524

514-
#[allow(clippy::unwrap_used)]
515-
let name_re = Regex::new(r"^[a-zA-Z0-9._-]+$").unwrap();
516525
if manifest.name.is_empty() {
517526
errors.push(ManifestValidationError {
518527
field: "name".to_string(),
519528
message: "name must not be empty".to_string(),
520529
});
521-
} else if !name_re.is_match(&manifest.name) {
530+
} else if !NAME_RE.is_match(&manifest.name) {
522531
errors.push(ManifestValidationError {
523532
field: "name".to_string(),
524533
message: "name must contain only alphanumeric characters, dashes, underscores, or dots"
525534
.to_string(),
526535
});
527536
}
528537

529-
#[allow(clippy::unwrap_used)]
530-
let version_re = Regex::new(r"^\d+\.\d+\.\d+$").unwrap();
531-
if !version_re.is_match(&manifest.version) {
538+
if !VERSION_RE.is_match(&manifest.version) {
532539
errors.push(ManifestValidationError {
533540
field: "version".to_string(),
534541
message: "version must match semver format X.Y.Z".to_string(),

src-tauri/src/extensions/utils.rs

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ pub fn extensions_directory_path() -> PathBuf {
1313
.join("extensions")
1414
}
1515

16-
/// Recursively copy a directory
16+
/// Recursively copy a directory, skipping symlinks to prevent symlink attacks.
1717
pub fn copy_dir_recursive(src: &PathBuf, dst: &PathBuf) -> std::io::Result<()> {
1818
if !dst.exists() {
1919
fs::create_dir_all(dst)?;
@@ -25,6 +25,10 @@ pub fn copy_dir_recursive(src: &PathBuf, dst: &PathBuf) -> std::io::Result<()> {
2525
let src_path = entry.path();
2626
let dst_path = dst.join(entry.file_name());
2727

28+
if file_type.is_symlink() {
29+
continue;
30+
}
31+
2832
if file_type.is_dir() {
2933
copy_dir_recursive(&src_path, &dst_path)?;
3034
} else {
@@ -35,6 +39,9 @@ pub fn copy_dir_recursive(src: &PathBuf, dst: &PathBuf) -> std::io::Result<()> {
3539
Ok(())
3640
}
3741

42+
const MAX_ZIP_ENTRIES: usize = 10_000;
43+
const MAX_ZIP_TOTAL_SIZE: u64 = 512 * 1024 * 1024; // 512 MB
44+
3845
/// Extract a zip package to a directory
3946
pub fn extract_zip_package(
4047
zip_path: &std::path::Path,
@@ -45,6 +52,25 @@ pub fn extract_zip_package(
4552
let mut archive =
4653
zip::ZipArchive::new(file).map_err(|e| format!("Failed to read zip archive: {}", e))?;
4754

55+
if archive.len() > MAX_ZIP_ENTRIES {
56+
return Err(format!(
57+
"Zip archive contains too many entries ({}, max {})",
58+
archive.len(),
59+
MAX_ZIP_ENTRIES
60+
));
61+
}
62+
63+
let total_size: u64 = (0..archive.len())
64+
.filter_map(|i| archive.by_index(i).ok())
65+
.map(|f| f.size())
66+
.sum();
67+
if total_size > MAX_ZIP_TOTAL_SIZE {
68+
return Err(format!(
69+
"Zip archive uncompressed size too large ({} bytes, max {} bytes)",
70+
total_size, MAX_ZIP_TOTAL_SIZE
71+
));
72+
}
73+
4874
fs::create_dir_all(target_dir)
4975
.map_err(|e| format!("Failed to create extraction directory: {}", e))?;
5076

src-tauri/src/extensions/wasm/host.rs

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,9 @@ fn validate_workspace_path(
330330
workspace_root: &str,
331331
relative_path: &str,
332332
) -> Result<std::path::PathBuf, String> {
333+
if relative_path.contains("..") {
334+
return Err("Path must not contain '..' components".to_string());
335+
}
333336
let root = Path::new(workspace_root)
334337
.canonicalize()
335338
.map_err(|e| format!("Invalid workspace root: {}", e))?;
@@ -597,7 +600,14 @@ pub fn host_save_document(uri: &str) -> Result<(), String> {
597600

598601
pub fn host_get_document_text_by_uri(uri: &str) -> Result<String, String> {
599602
debug!("[WasmExt] Get document text: uri='{}'", uri);
600-
fs::read_to_string(uri).map_err(|e| format!("Failed to read document '{}': {}", uri, e))
603+
if uri.is_empty() {
604+
return Err("Document URI must not be empty".to_string());
605+
}
606+
let path = Path::new(uri);
607+
if !path.is_absolute() {
608+
return Err(format!("Document URI must be an absolute path: '{}'", uri));
609+
}
610+
fs::read_to_string(path).map_err(|e| format!("Failed to read document '{}': {}", uri, e))
601611
}
602612

603613
pub fn host_document_line_at(uri: &str, line: u32) -> Result<String, String> {
@@ -1177,7 +1187,6 @@ static DIAGNOSTICS: Lazy<Mutex<Vec<DiagnosticEntry>>> = Lazy::new(|| Mutex::new(
11771187
static STATUS_BAR_MESSAGES: Lazy<Mutex<Vec<StatusBarEntry>>> = Lazy::new(|| Mutex::new(Vec::new()));
11781188
static CODE_ACTION_PROVIDERS: Lazy<Mutex<Vec<String>>> = Lazy::new(|| Mutex::new(Vec::new()));
11791189

1180-
#[allow(clippy::expect_used)]
11811190
pub fn host_register_language(language_id: &str, extensions_json: &str) {
11821191
let extensions: Vec<String> = serde_json::from_str(extensions_json).unwrap_or_default();
11831192
let entry = RegisteredLanguage {
@@ -1187,7 +1196,7 @@ pub fn host_register_language(language_id: &str, extensions_json: &str) {
11871196

11881197
let mut registry = REGISTERED_LANGUAGES
11891198
.lock()
1190-
.expect("Registered languages lock poisoned");
1199+
.unwrap_or_else(|e| e.into_inner());
11911200
registry.push(entry);
11921201

11931202
info!(
@@ -1196,7 +1205,6 @@ pub fn host_register_language(language_id: &str, extensions_json: &str) {
11961205
);
11971206
}
11981207

1199-
#[allow(clippy::expect_used)]
12001208
pub fn host_register_diagnostic(uri: &str, diagnostics: &str) {
12011209
let entry = DiagnosticEntry {
12021210
uri: uri.to_string(),
@@ -1205,18 +1213,17 @@ pub fn host_register_diagnostic(uri: &str, diagnostics: &str) {
12051213

12061214
let mut registry = DIAGNOSTICS
12071215
.lock()
1208-
.expect("Diagnostics registry lock poisoned");
1216+
.unwrap_or_else(|e| e.into_inner());
12091217
registry.retain(|d| d.uri != uri);
12101218
registry.push(entry);
12111219

12121220
info!("[WasmExt] Registered diagnostics for uri='{}'", uri);
12131221
}
12141222

1215-
#[allow(clippy::expect_used)]
12161223
pub fn host_register_code_action_provider(language_id: &str) {
12171224
let mut registry = CODE_ACTION_PROVIDERS
12181225
.lock()
1219-
.expect("Code action providers lock poisoned");
1226+
.unwrap_or_else(|e| e.into_inner());
12201227
if !registry.contains(&language_id.to_string()) {
12211228
registry.push(language_id.to_string());
12221229
}
@@ -1231,7 +1238,6 @@ pub fn host_get_workspace_path() -> Option<String> {
12311238
None
12321239
}
12331240

1234-
#[allow(clippy::expect_used)]
12351241
pub fn host_set_status_bar_message(text: &str, timeout_ms: Option<u32>) {
12361242
let created_at = SystemTime::now()
12371243
.duration_since(SystemTime::UNIX_EPOCH)
@@ -1245,7 +1251,7 @@ pub fn host_set_status_bar_message(text: &str, timeout_ms: Option<u32>) {
12451251

12461252
let mut registry = STATUS_BAR_MESSAGES
12471253
.lock()
1248-
.expect("Status bar messages lock poisoned");
1254+
.unwrap_or_else(|e| e.into_inner());
12491255
registry.push(entry);
12501256

12511257
info!(

src-tauri/src/extensions/wasm/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ pub async fn execute_wasm_command(
9595
.wasm_runtime
9696
.execute_command(&extension_id, &command, &args_json)?;
9797

98-
serde_json::from_str(&result).unwrap_or(Ok(serde_json::Value::String(result)))
98+
Ok(serde_json::from_str(&result).unwrap_or_else(|_| serde_json::Value::String(result)))
9999
}
100100

101101
#[cfg(not(feature = "wasm-extensions"))]

0 commit comments

Comments
 (0)