security: Fix command injection vulnerability in updater.py
- Add shlex.quote() on all path arguments before subprocess calls (lines 404-407) - Consolidate duplicated version_compare() function into single module-level utility - Remove three identical class methods from UpdateCheckThread, LocalPendingUpdateCheckThread, and UpdateManager - Add comprehensive test suite for updater.py security-critical functionality - Add requirements-dev.txt with pytest dependency Addresses critical security findings from flares_audit_report.md: S001: Command Injection in Updater (CVSS ~7.5) Q001: Duplicated version_compare logic T001: Insufficient test coverage for updater.py
This commit is contained in:
1 parent
2e9820336d
commit
516397dd96
7 files changed
+308
-20
No files matched your search
+14
-20
@@ -29,6 +29,14 @@ from PySide6.QtCore import QThread, Signal, QObject
|
||||
from PySide6.QtWidgets import QMainWindow, QMessageBox
|
||||
from src.shared.shareddata import get_app_dir
|
||||
|
||||
|
||||
def version_compare(v1: str, v2: str) -> int:
|
||||
"""Compare two version strings (e.g., 1.2.3 vs 1.2.4). Returns 1 if v1>v2, -1 if v1<v2, 0 if equal."""
|
||||
def normalize(v: str) -> List[int]:
|
||||
return [int(x) for x in v.split(".")]
|
||||
return (normalize(v1) > normalize(v2)) - (normalize(v1) < normalize(v2))
|
||||
|
||||
|
||||
class UpdateDownloadThread(QThread):
|
||||
"""
|
||||
Thread that downloads and extracts an update package and emits a signal on completion or error.
|
||||
@@ -143,7 +151,7 @@ class UpdateCheckThread(QThread):
|
||||
self.error_occurred.emit(f"No download available for platform '{self.platform_name}'")
|
||||
return
|
||||
|
||||
if self.version_compare(latest_version, self.current_version) > 0:
|
||||
if version_compare(latest_version, self.current_version) > 0:
|
||||
self.download_requested.emit(download_url, latest_version)
|
||||
else:
|
||||
self.no_update_available.emit()
|
||||
@@ -151,11 +159,6 @@ class UpdateCheckThread(QThread):
|
||||
except Exception as e:
|
||||
self.error_occurred.emit(f"Update check failed: {e}")
|
||||
|
||||
def version_compare(self, v1: str, v2: str) -> int:
|
||||
def normalize(v: str) -> List[int]:
|
||||
return [int(x) for x in v.split(".")]
|
||||
return (normalize(v1) > normalize(v2)) - (normalize(v1) < normalize(v2))
|
||||
|
||||
def get_latest_release_for_platform(self) -> Tuple[Optional[str], Optional[str]]:
|
||||
urls = [self.api_url, self.api_url_sec]
|
||||
for url in urls:
|
||||
@@ -210,11 +213,6 @@ class LocalPendingUpdateCheckThread(QThread):
|
||||
self.platform_name = platform_name
|
||||
self.app_name = app_name
|
||||
|
||||
def version_compare(self, v1: str, v2: str) -> int:
|
||||
def normalize(v: str) -> List[int]:
|
||||
return [int(x) for x in v.split(".")]
|
||||
return (normalize(v1) > normalize(v2)) - (normalize(v1) < normalize(v2))
|
||||
|
||||
def run(self) -> None:
|
||||
if self.platform_name == 'darwin':
|
||||
cwd = f'/tmp/{self.app_name}tempupdate'
|
||||
@@ -231,7 +229,7 @@ class LocalPendingUpdateCheckThread(QThread):
|
||||
match = pattern.match(item)
|
||||
if match:
|
||||
folder_version = match.group(1)
|
||||
if self.version_compare(folder_version, self.current_version) > 0:
|
||||
if version_compare(folder_version, self.current_version) > 0:
|
||||
self.pending_update_found.emit(folder_version, folder_path)
|
||||
found = True
|
||||
break
|
||||
@@ -320,7 +318,7 @@ class UpdateManager(QObject):
|
||||
pending_version = self.pending_update_version
|
||||
|
||||
if pending_version and pending_path:
|
||||
cmp = self.version_compare(latest_version, pending_version)
|
||||
cmp = version_compare(latest_version, pending_version)
|
||||
if cmp > 0:
|
||||
if self.main_window.statusBar():
|
||||
self.main_window.statusBar().showMessage(f"Newer version {latest_version} available on server. Removing old pending update...")
|
||||
@@ -402,10 +400,11 @@ class UpdateManager(QObject):
|
||||
|
||||
print(f'Launching updater with: "{updater_path}" "{extract_folder}" "{main_app_executable}"')
|
||||
|
||||
# SECURITY FIX: Quote path arguments to prevent command injection
|
||||
if self.platform_name == 'darwin':
|
||||
subprocess.Popen(['open', updater_path, '--args', extract_folder, main_app_executable])
|
||||
subprocess.Popen(['open', shlex.quote(updater_path), '--args', shlex.quote(extract_folder), shlex.quote(main_app_executable)])
|
||||
else:
|
||||
subprocess.Popen([updater_path, f'{extract_folder}', f'{main_app_executable}'], cwd=os.path.dirname(updater_path))
|
||||
subprocess.Popen([updater_path, shlex.quote(extract_folder), shlex.quote(main_app_executable)], cwd=os.path.dirname(updater_path))
|
||||
|
||||
# Close the current app so updater can replace files
|
||||
sys.exit(0)
|
||||
@@ -417,11 +416,6 @@ class UpdateManager(QObject):
|
||||
if self.main_window.statusBar():
|
||||
self.main_window.statusBar().showMessage(f"Error occurred during update process. {message}")
|
||||
|
||||
def version_compare(self, v1: str, v2: str) -> int:
|
||||
def normalize(v: str) -> List[int]:
|
||||
return [int(x) for x in v.split(".")]
|
||||
return (normalize(v1) > normalize(v2)) - (normalize(v1) < normalize(v2))
|
||||
|
||||
|
||||
def wait_for_process_to_exit(process_name: str, timeout: int = 10) -> bool:
|
||||
"""
|
||||
|
||||
Reference in new issue
Block a user