fix(cortex-update): address security vulnerabilities and clippy warnings

echobt · echobt · commit d65aa2b853ab · 2026-02-05T14:10:12.000Z
- Enforce HTTPS for update URLs (reject insecure URLs except localhost)
- Add InsecureUrl error variant for security violations
- Validate asset download URLs for HTTPS
- Fix TOCTOU race condition in archive extraction (single-pass)
- Collapse nested if statements
- Remove unneeded return statements
- Change &amp;PathBuf to &amp;Path parameter type
- Add #[allow(dead_code)] for utility functions
diff --git a/src/cortex-update/src/api.rs b/src/cortex-update/src/api.rs
@@ -90,28 +90,33 @@ impl CortexSoftwareClient {
 impl CortexSoftwareClient {
     /// Create a new client with the default URL.
     pub fn new() -> Self {
-        Self::with_url(SOFTWARE_URL.to_string())
+        // Default URL is known to be HTTPS, so unwrap is safe
+        Self::with_url(SOFTWARE_URL.to_string()).expect("Default SOFTWARE_URL must be HTTPS")
     }
 
     /// Create a new client with a custom URL.
-    pub fn with_url(base_url: String) -> Self {
+    ///
+    /// # Errors
+    ///
+    /// Returns `UpdateError::InsecureUrl` if the URL does not use HTTPS
+    /// (except for localhost/127.0.0.1 which are allowed for development).
+    pub fn with_url(base_url: String) -> UpdateResult<Self> {
         // Validate URL uses HTTPS for security (allow http for localhost development only)
         let url_lower = base_url.to_lowercase();
         if !url_lower.starts_with("https://")
             && !url_lower.starts_with("http://localhost")
             && !url_lower.starts_with("http://127.0.0.1")
         {
-            tracing::warn!(
-                "Custom update URL does not use HTTPS. This could allow man-in-the-middle attacks: {}",
-                base_url
-            );
+            return Err(UpdateError::InsecureUrl {
+                url: base_url,
+            });
         }
 
         let client = create_client_builder()
             .build()
             .unwrap_or_else(|_| Client::new());
 
-        Self { client, base_url }
+        Ok(Self { client, base_url })
     }
 
     /// Get the latest release for a channel.
@@ -216,6 +221,17 @@ impl CortexSoftwareClient {
     where
         F: FnMut(u64, u64), // (downloaded, total)
     {
+        // Validate asset URL uses HTTPS (allow localhost for development)
+        let url_lower = asset.url.to_lowercase();
+        if !url_lower.starts_with("https://")
+            && !url_lower.starts_with("http://localhost")
+            && !url_lower.starts_with("http://127.0.0.1")
+        {
+            return Err(UpdateError::InsecureUrl {
+                url: asset.url.clone(),
+            });
+        }
+
         let response =
             self.client
                 .get(&asset.url)
diff --git a/src/cortex-update/src/config.rs b/src/cortex-update/src/config.rs
@@ -106,17 +106,17 @@ impl UpdateConfig {
             .map(|h| h.join(".cortex").join("update.json"))
             .filter(|p| p.exists());
 
-        if let Some(path) = config_path {
-            if let Ok(content) = std::fs::read_to_string(&path) {
-                match serde_json::from_str(&content) {
-                    Ok(config) => return config,
-                    Err(e) => {
-                        tracing::warn!(
-                            "Failed to parse update config at {}: {}. Using defaults.",
-                            path.display(),
-                            e
-                        );
-                    }
+        if let Some(path) = config_path
+            && let Ok(content) = std::fs::read_to_string(&path)
+        {
+            match serde_json::from_str(&content) {
+                Ok(config) => return config,
+                Err(e) => {
+                    tracing::warn!(
+                        "Failed to parse update config at {}: {}. Using defaults.",
+                        path.display(),
+                        e
+                    );
                 }
             }
         }
diff --git a/src/cortex-update/src/download.rs b/src/cortex-update/src/download.rs
@@ -120,6 +120,7 @@ impl Downloader {
     }
 
     /// Download a signature file.
+    #[allow(dead_code)]
     pub async fn download_signature(&self, url: &str, version: &str) -> UpdateResult<PathBuf> {
         let filename = format!("{}.sig", version);
         let dest = self.temp_dir.join(filename);
@@ -131,11 +132,13 @@ impl Downloader {
     }
 
     /// Get the temp directory path.
+    #[allow(dead_code)]
     pub fn temp_dir(&self) -> &Path {
         &self.temp_dir
     }
 
     /// Clean up temp files.
+    #[allow(dead_code)]
     pub fn cleanup(&self) -> UpdateResult<()> {
         if self.temp_dir.exists() {
             std::fs::remove_dir_all(&self.temp_dir)?;
diff --git a/src/cortex-update/src/error.rs b/src/cortex-update/src/error.rs
@@ -90,6 +90,10 @@ pub enum UpdateError {
     // Cancelled
     #[error("Update cancelled by user")]
     Cancelled,
+
+    // Security errors
+    #[error("Insecure URL rejected (HTTPS required): {url}")]
+    InsecureUrl { url: String },
 }
 
 impl UpdateError {
diff --git a/src/cortex-update/src/install.rs b/src/cortex-update/src/install.rs
@@ -120,6 +120,8 @@ impl Installer {
     }
 
     /// Extract a tar.gz archive.
+    ///
+    /// Uses single-pass extraction with inline validation to prevent TOCTOU race conditions.
     async fn extract_tar_gz(&self, archive_path: &Path, dest: &Path) -> UpdateResult<PathBuf> {
         use flate2::read::GzDecoder;
         use std::fs::File;
@@ -129,13 +131,14 @@ impl Installer {
         let gz = GzDecoder::new(file);
         let mut archive = Archive::new(gz);
 
-        // Validate paths before extraction to prevent path traversal attacks
-        for entry in archive.entries().map_err(|e| UpdateError::ExtractionFailed {
+        // Single-pass: validate and extract each entry to prevent TOCTOU race conditions
+        for entry_result in archive.entries().map_err(|e| UpdateError::ExtractionFailed {
             message: e.to_string(),
         })? {
-            let entry = entry.map_err(|e| UpdateError::ExtractionFailed {
+            let mut entry = entry_result.map_err(|e| UpdateError::ExtractionFailed {
                 message: e.to_string(),
             })?;
+
             let path = entry.path().map_err(|e| UpdateError::ExtractionFailed {
                 message: e.to_string(),
             })?;
@@ -149,62 +152,79 @@ impl Installer {
                     message: format!("Path traversal detected in archive: {}", path.display()),
                 });
             }
-        }
-
-        // Re-open and extract after validation
-        let file = File::open(archive_path)?;
-        let gz = GzDecoder::new(file);
-        let mut archive = Archive::new(gz);
 
-        archive
-            .unpack(dest)
-            .map_err(|e| UpdateError::ExtractionFailed {
-                message: e.to_string(),
-            })?;
+            // Extract the entry immediately after validation
+            entry
+                .unpack_in(dest)
+                .map_err(|e| UpdateError::ExtractionFailed {
+                    message: e.to_string(),
+                })?;
+        }
 
         self.find_binary(dest).await
     }
 
     /// Extract a zip archive.
+    ///
+    /// Uses single-pass extraction with inline validation to prevent TOCTOU race conditions.
     async fn extract_zip(&self, archive_path: &Path, dest: &Path) -> UpdateResult<PathBuf> {
         let file = std::fs::File::open(archive_path)?;
         let mut archive =
             zip::ZipArchive::new(file).map_err(|e| UpdateError::ExtractionFailed {
                 message: e.to_string(),
             })?;
 
-        // Validate all paths before extraction to prevent path traversal attacks
+        // Single-pass: validate and extract each entry to prevent TOCTOU race conditions
         for i in 0..archive.len() {
-            let file = archive
+            let mut zip_file = archive
                 .by_index(i)
                 .map_err(|e| UpdateError::ExtractionFailed {
                     message: e.to_string(),
                 })?;
-            let path = std::path::Path::new(file.name());
+
+            let path = std::path::Path::new(zip_file.name());
 
             // Check for path traversal
             if path
                 .components()
                 .any(|c| matches!(c, std::path::Component::ParentDir))
             {
                 return Err(UpdateError::ExtractionFailed {
-                    message: format!("Path traversal detected in archive: {}", file.name()),
+                    message: format!("Path traversal detected in archive: {}", zip_file.name()),
                 });
             }
-        }
 
-        // Re-open and extract after validation
-        let file = std::fs::File::open(archive_path)?;
-        let mut archive =
-            zip::ZipArchive::new(file).map_err(|e| UpdateError::ExtractionFailed {
-                message: e.to_string(),
-            })?;
+            // Determine the output path
+            let outpath = dest.join(zip_file.name());
 
-        archive
-            .extract(dest)
-            .map_err(|e| UpdateError::ExtractionFailed {
-                message: e.to_string(),
-            })?;
+            // Extract the entry immediately after validation
+            if zip_file.is_dir() {
+                std::fs::create_dir_all(&outpath)?;
+            } else {
+                // Ensure parent directory exists
+                if let Some(parent) = outpath.parent() {
+                    std::fs::create_dir_all(parent)?;
+                }
+                let mut outfile =
+                    std::fs::File::create(&outpath).map_err(|e| UpdateError::ExtractionFailed {
+                        message: format!("Failed to create file {}: {}", outpath.display(), e),
+                    })?;
+                std::io::copy(&mut zip_file, &mut outfile).map_err(|e| {
+                    UpdateError::ExtractionFailed {
+                        message: format!("Failed to extract {}: {}", outpath.display(), e),
+                    }
+                })?;
+
+                // Set file permissions on Unix
+                #[cfg(unix)]
+                {
+                    use std::os::unix::fs::PermissionsExt;
+                    if let Some(mode) = zip_file.unix_mode() {
+                        std::fs::set_permissions(&outpath, std::fs::Permissions::from_mode(mode))?;
+                    }
+                }
+            }
+        }
 
         self.find_binary(dest).await
     }
@@ -328,6 +348,7 @@ impl Installer {
 }
 
 /// Check if we have permission to write to the installation directory.
+#[allow(dead_code)]
 pub fn check_write_permission() -> UpdateResult<()> {
     let exe_path = std::env::current_exe()?;
     let parent = exe_path
diff --git a/src/cortex-update/src/manager.rs b/src/cortex-update/src/manager.rs
@@ -1,7 +1,5 @@
 //! Update manager - main API for update operations.
 
-use std::path::PathBuf;
-
 use crate::CURRENT_VERSION;
 use crate::api::{CortexSoftwareClient, ReleaseAsset, ReleaseInfo};
 use crate::config::{ReleaseChannel, UpdateConfig};
@@ -61,7 +59,7 @@ impl UpdateManager {
     /// Create with a specific config.
     pub fn with_config(config: UpdateConfig) -> UpdateResult<Self> {
         let client = if let Some(url) = &config.custom_url {
-            CortexSoftwareClient::with_url(url.clone())
+            CortexSoftwareClient::with_url(url.clone())?
         } else {
             CortexSoftwareClient::new()
         };
@@ -89,10 +87,12 @@ impl UpdateManager {
     pub async fn check_update(&self) -> UpdateResult<Option<UpdateInfo>> {
         // Try to use cache first
         if let Some(cache) = VersionCache::load() {
-            if cache.is_valid(&self.config) {
-                if cache.has_update() && !self.config.is_version_skipped(&cache.latest.version) {
-                    return Ok(Some(self.build_update_info(&cache.latest)?));
-                }
+            if cache.is_valid(&self.config)
+                && cache.has_update()
+                && !self.config.is_version_skipped(&cache.latest.version)
+            {
+                return Ok(Some(self.build_update_info(&cache.latest)?));
+            } else if cache.is_valid(&self.config) {
                 return Ok(None);
             }
         }
diff --git a/src/cortex-update/src/method.rs b/src/cortex-update/src/method.rs
@@ -1,6 +1,8 @@
 //! Installation method detection.
 
 use serde::{Deserialize, Serialize};
+use std::path::Path;
+#[cfg(test)]
 use std::path::PathBuf;
 
 /// Installation method for Cortex CLI.
@@ -27,10 +29,10 @@ impl InstallMethod {
     /// Detect the installation method based on environment and paths.
     pub fn detect() -> Self {
         // 1. Check executable path first
-        if let Ok(exe_path) = std::env::current_exe() {
-            if let Some(method) = Self::detect_from_path(&exe_path) {
-                return method;
-            }
+        if let Ok(exe_path) = std::env::current_exe()
+            && let Some(method) = Self::detect_from_path(&exe_path)
+        {
+            return method;
         }
 
         // 2. Platform-specific defaults
@@ -40,7 +42,7 @@ impl InstallMethod {
             if which_exists("winget") {
                 return Self::WinGet;
             }
-            return Self::PowerShellScript;
+            Self::PowerShellScript
         }
 
         #[cfg(not(windows))]
@@ -49,12 +51,12 @@ impl InstallMethod {
             if which_exists("brew") {
                 return Self::Homebrew;
             }
-            return Self::CurlScript;
+            Self::CurlScript
         }
     }
 
     /// Detect from executable path.
-    fn detect_from_path(path: &PathBuf) -> Option<Self> {
+    fn detect_from_path(path: &Path) -> Option<Self> {
         let path_str = path.to_string_lossy().to_lowercase();
 
         // Homebrew paths (macOS and Linux)
diff --git a/src/cortex-update/src/verify.rs b/src/cortex-update/src/verify.rs
@@ -34,6 +34,7 @@ pub async fn verify_sha256(path: &Path, expected: &str) -> UpdateResult<()> {
 }
 
 /// Verify SHA256 checksum synchronously (for smaller files).
+#[allow(dead_code)]
 pub fn verify_sha256_sync(path: &Path, expected: &str) -> UpdateResult<()> {
     let content = std::fs::read(path)?;
     let result = Sha256::digest(&content);
@@ -49,6 +50,7 @@ pub fn verify_sha256_sync(path: &Path, expected: &str) -> UpdateResult<()> {
 }
 
 /// Calculate SHA256 hash of a file.
+#[allow(dead_code)]
 pub async fn calculate_sha256(path: &Path) -> UpdateResult<String> {
     let mut file = tokio::fs::File::open(path).await?;
     let mut hasher = Sha256::new();
diff --git a/src/cortex-update/src/version.rs b/src/cortex-update/src/version.rs
@@ -125,6 +125,7 @@ fn parse_version(version: &str) -> (u32, u32, u32, String) {
 }
 
 /// Check if a version meets the minimum requirement.
+#[allow(dead_code)]
 pub fn meets_minimum(current: &str, minimum: &str) -> bool {
     compare_versions(current, minimum) != VersionComparison::Older
 }

Original file line number	Diff line number	Diff line change
`@@ -120,6 +120,7 @@ impl Downloader {`
`120`	`120`	`}`
`121`	`121`
`122`	`122`	`/// Download a signature file.`
	`123`	`+ #[allow(dead_code)]`
`123`	`124`	`pub async fn download_signature(&self, url: &str, version: &str) -> UpdateResult<PathBuf> {`
`124`	`125`	`let filename = format!("{}.sig", version);`
`125`	`126`	`let dest = self.temp_dir.join(filename);`
`@@ -131,11 +132,13 @@ impl Downloader {`
`131`	`132`	`}`
`132`	`133`
`133`	`134`	`/// Get the temp directory path.`
	`135`	`+ #[allow(dead_code)]`
`134`	`136`	`pub fn temp_dir(&self) -> &Path {`
`135`	`137`	`&self.temp_dir`
`136`	`138`	`}`
`137`	`139`
`138`	`140`	`/// Clean up temp files.`
	`141`	`+ #[allow(dead_code)]`
`139`	`142`	`pub fn cleanup(&self) -> UpdateResult<()> {`
`140`	`143`	`if self.temp_dir.exists() {`
`141`	`144`	`std::fs::remove_dir_all(&self.temp_dir)?;`
Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,7 @@ pub async fn verify_sha256(path: &Path, expected: &str) -> UpdateResult<()> {`
`34`	`34`	`}`
`35`	`35`
`36`	`36`	`/// Verify SHA256 checksum synchronously (for smaller files).`
	`37`	`+#[allow(dead_code)]`
`37`	`38`	`pub fn verify_sha256_sync(path: &Path, expected: &str) -> UpdateResult<()> {`
`38`	`39`	`let content = std::fs::read(path)?;`
`39`	`40`	`let result = Sha256::digest(&content);`
`@@ -49,6 +50,7 @@ pub fn verify_sha256_sync(path: &Path, expected: &str) -> UpdateResult<()> {`
`49`	`50`	`}`
`50`	`51`
`51`	`52`	`/// Calculate SHA256 hash of a file.`
	`53`	`+#[allow(dead_code)]`
`52`	`54`	`pub async fn calculate_sha256(path: &Path) -> UpdateResult<String> {`
`53`	`55`	`let mut file = tokio::fs::File::open(path).await?;`
`54`	`56`	`let mut hasher = Sha256::new();`
Original file line number	Diff line number	Diff line change
`@@ -125,6 +125,7 @@ fn parse_version(version: &str) -> (u32, u32, u32, String) {`
`125`	`125`	`}`
`126`	`126`
`127`	`127`	`/// Check if a version meets the minimum requirement.`
	`128`	`+#[allow(dead_code)]`
`128`	`129`	`pub fn meets_minimum(current: &str, minimum: &str) -> bool {`
`129`	`130`	`compare_versions(current, minimum) != VersionComparison::Older`
`130`	`131`	`}`