Fixed build issues & view loading#48
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughUpdates version handling in CI/CD workflows to separate numeric/build components. Refactors GUI views with thread-safe async UI update mechanisms via new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WingetModule as Winget Module
participant PowerShell
participant WingetAPI as Winget API
participant WingetCLI as Winget CLI
Client->>WingetModule: search_packages()
activate WingetModule
WingetModule->>PowerShell: _ensure_winget_module()
alt Microsoft.WinGet.Client Available
PowerShell-->>WingetModule: Module Ready
WingetModule->>PowerShell: _search_via_powershell()
alt PowerShell Search Success
PowerShell-->>WingetModule: JSON Results
WingetModule-->>Client: Package List
else PowerShell Search Fails
WingetModule->>WingetAPI: Fallback to API
WingetAPI-->>WingetModule: Results
WingetModule-->>Client: Package List
end
else Module Not Available
PowerShell-->>WingetModule: Install Attempt
alt Module Install Succeeds
WingetModule->>PowerShell: Retry Search
PowerShell-->>WingetModule: Results
else Module Install Fails
WingetModule->>WingetCLI: Fallback to CLI
WingetCLI-->>WingetModule: CLI Output
end
WingetModule-->>Client: Results
end
deactivate WingetModule
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/switchcraft_winget/utils/winget.py (3)
130-147: Command injection vulnerability via unsanitized query input.The
queryparameter is interpolated directly into a PowerShell command string without sanitization. If a user searches for a query containing single quotes or PowerShell escape sequences (e.g.,test'; Remove-Item -Recurse C:\; '), it could execute arbitrary commands.Consider using parameterized PowerShell input or sanitizing the query:
🔒 Proposed fix using sanitization
def _search_via_powershell(self, query: str) -> List[Dict[str, str]]: ... try: # Ensure module is available (but don't fail if it's not - we have fallbacks) self._ensure_winget_module() - - ps_script = f"Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue; Find-WinGetPackage -Query '{query}' | Select-Object Name, Id, Version, Source | ConvertTo-Json -Depth 1" + + # Sanitize query to prevent command injection + safe_query = query.replace("'", "''").replace("`", "``") + ps_script = f"Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue; Find-WinGetPackage -Query '{safe_query}' | Select-Object Name, Id, Version, Source | ConvertTo-Json -Depth 1"
352-367: CLI install fallback missing timeout.The PowerShell install path has a 300-second timeout (line 658), but the CLI fallback
subprocess.runon line 363 has no timeout. A stuck installation could hang the application indefinitely.🐛 Proposed fix
- proc = subprocess.run(cmd, capture_output=True, text=True, **kwargs) + proc = subprocess.run(cmd, capture_output=True, text=True, timeout=300, **kwargs) if proc.returncode != 0: logger.error(f"Winget install failed: {proc.stderr}") return False return True + except subprocess.TimeoutExpired: + logger.error(f"Winget CLI install timed out for package: {package_id}") + return False except Exception as e:
384-398: CLI download fallback missing timeout.Same issue: the
subprocess.runon line 395 has no timeout, potentially causing the application to hang if the download stalls.🐛 Proposed fix
- proc = subprocess.run(cmd, capture_output=True, text=True, **kwargs) + proc = subprocess.run(cmd, capture_output=True, text=True, timeout=300, **kwargs) if proc.returncode != 0:Add timeout exception handling similar to
get_package_details.src/switchcraft/gui_modern/views/settings_view.py (1)
1749-1856: GPO cert path is detected but not honoredIf policy sets
CodeSigningCertPath(without a thumbprint), the method still proceeds to auto-detect and writes user prefs. That can surface a non‑policy cert in UI and store conflicting preferences. Consider short‑circuiting when a GPO path exists and skipping preference writes whenever either GPO value is set.✅ Suggested fix
- gpo_thumb = SwitchCraftConfig.get_value("CodeSigningCertThumbprint", "") - gpo_cert_path = SwitchCraftConfig.get_value("CodeSigningCertPath", "") + gpo_thumb = SwitchCraftConfig.get_value("CodeSigningCertThumbprint", "") + gpo_cert_path = SwitchCraftConfig.get_value("CodeSigningCertPath", "") + import os ... - if gpo_thumb or gpo_cert_path: + if gpo_thumb or gpo_cert_path: try: if gpo_thumb: ... if verify_proc.returncode == 0 and "FOUND" in verify_proc.stdout: ... return + if gpo_cert_path: + if os.path.exists(gpo_cert_path): + self.cert_status_text.value = f"GPO: {gpo_cert_path}" + self.cert_status_text.color = "GREEN" + self.update() + self._show_snack(i18n.get("cert_gpo_detected") or "GPO-configured certificate detected.", "GREEN") + return except Exception as ex: logger.debug(f"GPO cert verification failed: {ex}") # Continue with auto-detection if verification fails ... - if not gpo_thumb: + if not (gpo_thumb or gpo_cert_path): SwitchCraftConfig.set_user_preference("CodeSigningCertThumbprint", thumb) SwitchCraftConfig.set_user_preference("CodeSigningCertPath", "") ... - if not gpo_thumb: + if not (gpo_thumb or gpo_cert_path): SwitchCraftConfig.set_user_preference("CodeSigningCertThumbprint", thumb)
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 188-200: The sed replacements that set MyAppVersion,
MyAppVersionNumeric, and MyAppVersionInfo in both switchcraft_modern.iss and
switchcraft_legacy.iss should preserve any leading indentation; update each sed
invocation that currently matches "#define MyAppVersion", "#define
MyAppVersionNumeric" and "#define MyAppVersionInfo" to capture and reuse leading
whitespace (e.g., a capture for ^[[:space:]]*) so the replacement reinserts the
same indentation while substituting DEV_VERSION, BASE_VERSION and VERSION_INFO
respectively for the six occurrences in the diff.
In `@src/switchcraft_winget/utils/winget.py`:
- Around line 664-702: The _download_via_powershell function is misleading
because it never downloads and always returns None; either remove the function
or rename it (e.g., _verify_package_exists_via_powershell) and adjust callers
(like download_package) accordingly so behavior is clear; if you keep the
existence check, ensure it actually affects control flow (return a boolean/raise
on not found) and avoid command injection by not interpolating package_id into
the PowerShell string—pass it as a safe argument or validate/escape it before
use, and update any logic that expects a Path (e.g., callers of
_download_via_powershell) to handle the new return type.
- Around line 536-573: In _ensure_winget_module, stop silently forcing an
install: add a configuration flag (e.g. self.auto_install_winget or a param) and
only attempt the Install-Module block when that flag is True; if the flag is
False, skip running the ps_script install branch and immediately return False so
the CLI fallback is used. If you keep auto-installation enabled, log an info
message before attempting installation (logger.info(...)) and after a successful
install (detect "INSTALLED" in proc.stdout and logger.info("Automatically
installed Microsoft.WinGet.Client")). Update the function to reference the
existing ps_script, subprocess.run call and proc.stdout/proc.returncode checks
to implement the opt-in behavior and additional logs.
- Around line 582-588: The PowerShell snippet in ps_script interpolates
package_id directly (vulnerable to command injection), so change the script to
accept a parameter (e.g., param($id)) and reference $id inside the script
instead of inserting package_id into the string, and invoke the PowerShell
process with an ArgumentList (or equivalent API) to pass package_id as a
separate argument; do the same pattern used in _search_via_powershell (or adapt
that secure approach) so package_id is not concatenated into ps_script.
- Around line 637-646: The PS script builds a command using the unsanitized
package_id (ps_script / Install-WinGetPackage) leading to command injection;
sanitize or validate package_id the same way you handled other inputs (e.g.,
escape/whitelist or use a safe helper function used elsewhere) before
interpolating it into ps_script, and ensure the sanitized value is used in the
Install-WinGetPackage -Id parameter and any related string construction.
In `@src/switchcraft/gui_modern/views/winget_view.py`:
- Around line 372-384: The code merges short_info and full without checking
full, which can be None and cause a TypeError; update the block after calling
self.winget.get_package_details(short_info['Id']) (the get_package_details call)
to validate/coerce full (e.g., if full is None: log a warning and set full = {}
or raise immediately) before running merged = {**short_info, **full}, then set
self.current_pkg = merged and keep the existing logger messages; ensure the
existing check that raises on empty details still runs after the validation or
replace it with the immediate None-handling logic.
🧹 Nitpick comments (7)
scripts/build_release.ps1 (1)
157-161: Consider extracting version parsing into a helper function.The version extraction logic (regex matching, numeric extraction, and
AppVersionInfoconstruction) is duplicated between the fallback block (lines 157-161) and the successful extraction block (lines 168-173). While functional, this creates a maintenance burden if the parsing logic needs to change.♻️ Suggested refactor
+# Helper function to extract version components +function Get-VersionComponents { + param([string]$Version) + $Numeric = if ($Version -match '^(\d+\.\d+\.\d+)') { $Matches[1] } else { $Version -replace '[^0-9.].*', '' } + return @{ + Full = $Version + Numeric = $Numeric + Info = "$Numeric.0" + } +} + # --- Version Extraction --- $PyProjectFile = Join-Path $RepoRoot "pyproject.toml" # Fallback version if extraction fails (can be overridden via env variable) $AppVersion = if ($env:SWITCHCRAFT_VERSION) { $env:SWITCHCRAFT_VERSION } else { "2026.1.2" } -# Extract numeric version only (remove .dev0, +build, -dev, etc.) for VersionInfoVersion -# Pattern: extract MAJOR.MINOR.PATCH from any version format -$AppVersionNumeric = if ($AppVersion -match '^(\d+\.\d+\.\d+)') { $Matches[1] } else { $AppVersion -replace '[^0-9.].*', '' } -# VersionInfoVersion requires 4 numeric components (Major.Minor.Patch.Build) -$AppVersionInfo = "$AppVersionNumeric.0" +$VersionParts = Get-VersionComponents -Version $AppVersion +$AppVersionNumeric = $VersionParts.Numeric +$AppVersionInfo = $VersionParts.Info if (Test-Path $PyProjectFile) { try { $VersionLine = Get-Content -Path $PyProjectFile | Select-String "version = " | Select-Object -First 1 if ($VersionLine -match 'version = "(.*)"') { $AppVersion = $Matches[1] - # Extract numeric version only (remove .dev0, +build, -dev, etc.) for VersionInfoVersion - # Pattern: extract MAJOR.MINOR.PATCH from any version format - $AppVersionNumeric = if ($AppVersion -match '^(\d+\.\d+\.\d+)') { $Matches[1] } else { $AppVersion -replace '[^0-9.].*', '' } - # VersionInfoVersion requires 4 numeric components (Major.Minor.Patch.Build) - $AppVersionInfo = "$AppVersionNumeric.0" + $VersionParts = Get-VersionComponents -Version $AppVersion + $AppVersionNumeric = $VersionParts.Numeric + $AppVersionInfo = $VersionParts.Info Write-Host "Detected Version: $AppVersion (Numeric base: $AppVersionNumeric, Info: $AppVersionInfo)" -ForegroundColor CyanAlso applies to: 168-173
src/switchcraft_winget/utils/winget.py (1)
136-146: Consider extracting repeated subprocess kwargs setup into a helper.The pattern for building subprocess kwargs with
startupinfoandcreationflagsis duplicated ~8 times throughout this file. Additionally,import sysappears inside multiple functions.♻️ Suggested helper method
def _get_subprocess_kwargs(self) -> dict: """Build subprocess kwargs for hidden console window on Windows.""" kwargs = {} startupinfo = self._get_startup_info() if startupinfo: kwargs['startupinfo'] = startupinfo if sys.platform == "win32": kwargs['creationflags'] = getattr(subprocess, 'CREATE_NO_WINDOW', 0x08000000) return kwargsThen replace all instances with:
kwargs = self._get_subprocess_kwargs() proc = subprocess.run(cmd, capture_output=True, text=True, ..., **kwargs)Also move
import systo the module level (line 1-8 area).src/switchcraft/gui_modern/utils/view_utils.py (1)
57-99: Add a direct-call fallback whenrun_taskis unavailable.At Line 79-81,
_run_task_safereturnsFalsewithout executingfuncifpageorrun_taskis missing, which can silently skip UI updates (e.g., tests or older Flet builds). Consider falling back to a direct call to preserve behavior.💡 Suggested change
- if not page or not hasattr(page, 'run_task'): - return False + if not page or not hasattr(page, 'run_task'): + try: + func() + return True + except Exception as ex: + logger.debug(f"Failed to run task directly: {ex}") + return Falsesrc/switchcraft/gui_modern/views/winget_view.py (1)
463-488: Consider delegating toViewMixin._run_task_safefor consistencyThis logic mirrors the shared helper and is likely to drift over time. Centralizing would keep UI marshaling consistent across views.
♻️ Possible direction
- import inspect - import asyncio - - # Check if function is async - is_async = inspect.iscoroutinefunction(ui_func) - - if hasattr(self, 'app_page') and hasattr(self.app_page, 'run_task'): - try: - if is_async: - self.app_page.run_task(ui_func) - logger.debug("UI update scheduled via run_task (async)") - else: - async def async_wrapper(): - ui_func() - self.app_page.run_task(async_wrapper) - logger.debug("UI update scheduled via run_task (sync wrapped)") - except Exception as ex: - ... - else: - ... + # Reuse shared helper to keep behavior consistent across views + if not self._run_task_safe(ui_func): + logger.debug("UI update executed directly (no run_task available)")src/switchcraft/gui_modern/views/intune_store_view.py (1)
214-227: Consolidate run_task wrapping viaViewMixin._run_task_safeThis wrapper pattern is repeated across several call sites. Using the shared helper would reduce duplication and keep marshaling behavior consistent across views.
Also applies to: 315-322, 384-387, 607-613
src/switchcraft/gui_modern/views/group_manager_view.py (1)
489-491: Redundanthasattr(run_task)guard around_run_task_safe
_run_task_safealready handles the absence ofrun_taskand falls back; you can call it unconditionally here.src/switchcraft/gui_modern/views/settings_view.py (1)
1521-1554: Extract addon asset selection into a shared helperThe asset-matching logic is now duplicated in two functions. A small helper (e.g.,
_select_addon_asset(assets, addon_id)) would reduce drift and keep the search rules consistent.Also applies to: 1594-1617, 1645-1649
Comment on lines
+188
to
+200
| # Update .iss files (Inno Setup installer scripts) | ||
| # Extract numeric version only (remove .dev0, +build, etc.) for VersionInfoVersion | ||
| BASE_VERSION=$(echo "$DEV_VERSION" | sed -E 's/([0-9]+\.[0-9]+\.[0-9]+).*/\1/') | ||
| # VersionInfoVersion requires 4 numeric components (Major.Minor.Patch.Build) | ||
| VERSION_INFO="${BASE_VERSION}.0" | ||
| # For dev releases: MyAppVersion should include commit ID (full DEV_VERSION) | ||
| # MyAppVersionNumeric should be numeric only (BASE_VERSION) | ||
| sed -i "s/#define MyAppVersion \".*\"/#define MyAppVersion \"$DEV_VERSION\"/" switchcraft_modern.iss | ||
| sed -i "s/#define MyAppVersionNumeric \".*\"/#define MyAppVersionNumeric \"$BASE_VERSION\"/" switchcraft_modern.iss | ||
| sed -i "s/#define MyAppVersion \".*\"/#define MyAppVersion \"$BASE_VERSION\"/" switchcraft_legacy.iss | ||
| sed -i "s/#define MyAppVersionInfo \".*\"/#define MyAppVersionInfo \"$VERSION_INFO\"/" switchcraft_modern.iss | ||
| sed -i "s/#define MyAppVersion \".*\"/#define MyAppVersion \"$DEV_VERSION\"/" switchcraft_legacy.iss | ||
| sed -i "s/#define MyAppVersionNumeric \".*\"/#define MyAppVersionNumeric \"$BASE_VERSION\"/" switchcraft_legacy.iss | ||
| sed -i "s/#define MyAppVersionInfo \".*\"/#define MyAppVersionInfo \"$VERSION_INFO\"/" switchcraft_legacy.iss |
Contributor
There was a problem hiding this comment.
Same indentation concern applies here.
The sed patterns in this step have the same potential issue with indented #define lines as noted above. Apply the same fix with \(^[[:space:]]*\) capture group to preserve indentation.
🤖 Prompt for AI Agents
In @.github/workflows/release.yml around lines 188 - 200, The sed replacements
that set MyAppVersion, MyAppVersionNumeric, and MyAppVersionInfo in both
switchcraft_modern.iss and switchcraft_legacy.iss should preserve any leading
indentation; update each sed invocation that currently matches "#define
MyAppVersion", "#define MyAppVersionNumeric" and "#define MyAppVersionInfo" to
capture and reuse leading whitespace (e.g., a capture for ^[[:space:]]*) so the
replacement reinserts the same indentation while substituting DEV_VERSION,
BASE_VERSION and VERSION_INFO respectively for the six occurrences in the diff.
Comment on lines
+536
to
+573
| def _ensure_winget_module(self) -> bool: | ||
| """ | ||
| Ensure Microsoft.WinGet.Client module is available. | ||
| Returns True if module is available or successfully installed, False otherwise. | ||
| """ | ||
| try: | ||
| ps_script = """ | ||
| if (-not (Get-Module -ListAvailable -Name Microsoft.WinGet.Client)) { | ||
| try { | ||
| Install-Module -Name Microsoft.WinGet.Client -Scope CurrentUser -Force -AllowClobber -ErrorAction Stop | ||
| Write-Output "INSTALLED" | ||
| } catch { | ||
| Write-Output "FAILED: $_" | ||
| exit 1 | ||
| } | ||
| } else { | ||
| Write-Output "AVAILABLE" | ||
| } | ||
| """ | ||
| cmd = ["powershell", "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", "-Command", ps_script] | ||
| startupinfo = self._get_startup_info() | ||
| kwargs = {} | ||
| if startupinfo: | ||
| kwargs['startupinfo'] = startupinfo | ||
| import sys | ||
| if sys.platform == "win32": | ||
| if hasattr(subprocess, 'CREATE_NO_WINDOW'): | ||
| kwargs['creationflags'] = subprocess.CREATE_NO_WINDOW | ||
| else: | ||
| kwargs['creationflags'] = 0x08000000 | ||
| proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="ignore", timeout=60, **kwargs) | ||
| if proc.returncode == 0 and ("AVAILABLE" in proc.stdout or "INSTALLED" in proc.stdout): | ||
| return True | ||
| logger.warning(f"WinGet module check failed: {proc.stderr}") | ||
| return False | ||
| except Exception as e: | ||
| logger.debug(f"WinGet module check exception: {e}") | ||
| return False |
Contributor
There was a problem hiding this comment.
Auto-installing PowerShell module without user consent.
The code silently installs Microsoft.WinGet.Client with -Force -AllowClobber if not present. This modifies the user's system without explicit consent, which may be unexpected behavior. Consider either:
- Logging an info message before attempting installation
- Making module installation opt-in via a configuration flag
- Simply returning False and relying on CLI fallback when the module is unavailable
💡 Suggested approach: Log and let user know
if (-not (Get-Module -ListAvailable -Name Microsoft.WinGet.Client)) {
try {
+ Write-Warning "Installing Microsoft.WinGet.Client module..."
Install-Module -Name Microsoft.WinGet.Client -Scope CurrentUser -Force -AllowClobber -ErrorAction Stop
Write-Output "INSTALLED"Also consider adding a Python-side log:
if "INSTALLED" in proc.stdout:
logger.info("Automatically installed Microsoft.WinGet.Client PowerShell module")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _ensure_winget_module(self) -> bool: | |
| """ | |
| Ensure Microsoft.WinGet.Client module is available. | |
| Returns True if module is available or successfully installed, False otherwise. | |
| """ | |
| try: | |
| ps_script = """ | |
| if (-not (Get-Module -ListAvailable -Name Microsoft.WinGet.Client)) { | |
| try { | |
| Install-Module -Name Microsoft.WinGet.Client -Scope CurrentUser -Force -AllowClobber -ErrorAction Stop | |
| Write-Output "INSTALLED" | |
| } catch { | |
| Write-Output "FAILED: $_" | |
| exit 1 | |
| } | |
| } else { | |
| Write-Output "AVAILABLE" | |
| } | |
| """ | |
| cmd = ["powershell", "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", "-Command", ps_script] | |
| startupinfo = self._get_startup_info() | |
| kwargs = {} | |
| if startupinfo: | |
| kwargs['startupinfo'] = startupinfo | |
| import sys | |
| if sys.platform == "win32": | |
| if hasattr(subprocess, 'CREATE_NO_WINDOW'): | |
| kwargs['creationflags'] = subprocess.CREATE_NO_WINDOW | |
| else: | |
| kwargs['creationflags'] = 0x08000000 | |
| proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="ignore", timeout=60, **kwargs) | |
| if proc.returncode == 0 and ("AVAILABLE" in proc.stdout or "INSTALLED" in proc.stdout): | |
| return True | |
| logger.warning(f"WinGet module check failed: {proc.stderr}") | |
| return False | |
| except Exception as e: | |
| logger.debug(f"WinGet module check exception: {e}") | |
| return False | |
| def _ensure_winget_module(self) -> bool: | |
| """ | |
| Ensure Microsoft.WinGet.Client module is available. | |
| Returns True if module is available or successfully installed, False otherwise. | |
| """ | |
| try: | |
| ps_script = """ | |
| if (-not (Get-Module -ListAvailable -Name Microsoft.WinGet.Client)) { | |
| try { | |
| Write-Warning "Installing Microsoft.WinGet.Client module..." | |
| Install-Module -Name Microsoft.WinGet.Client -Scope CurrentUser -Force -AllowClobber -ErrorAction Stop | |
| Write-Output "INSTALLED" | |
| } catch { | |
| Write-Output "FAILED: $_" | |
| exit 1 | |
| } | |
| } else { | |
| Write-Output "AVAILABLE" | |
| } | |
| """ | |
| cmd = ["powershell", "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", "-Command", ps_script] | |
| startupinfo = self._get_startup_info() | |
| kwargs = {} | |
| if startupinfo: | |
| kwargs['startupinfo'] = startupinfo | |
| import sys | |
| if sys.platform == "win32": | |
| if hasattr(subprocess, 'CREATE_NO_WINDOW'): | |
| kwargs['creationflags'] = subprocess.CREATE_NO_WINDOW | |
| else: | |
| kwargs['creationflags'] = 0x08000000 | |
| proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="ignore", timeout=60, **kwargs) | |
| if proc.returncode == 0 and ("AVAILABLE" in proc.stdout or "INSTALLED" in proc.stdout): | |
| return True | |
| logger.warning(f"WinGet module check failed: {proc.stderr}") | |
| return False | |
| except Exception as e: | |
| logger.debug(f"WinGet module check exception: {e}") | |
| return False |
🤖 Prompt for AI Agents
In `@src/switchcraft_winget/utils/winget.py` around lines 536 - 573, In
_ensure_winget_module, stop silently forcing an install: add a configuration
flag (e.g. self.auto_install_winget or a param) and only attempt the
Install-Module block when that flag is True; if the flag is False, skip running
the ps_script install branch and immediately return False so the CLI fallback is
used. If you keep auto-installation enabled, log an info message before
attempting installation (logger.info(...)) and after a successful install
(detect "INSTALLED" in proc.stdout and logger.info("Automatically installed
Microsoft.WinGet.Client")). Update the function to reference the existing
ps_script, subprocess.run call and proc.stdout/proc.returncode checks to
implement the opt-in behavior and additional logs.
Comment on lines
+582
to
+588
| ps_script = f""" | ||
| Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue | ||
| $pkg = Get-WinGetPackage -Id '{package_id}' -ErrorAction SilentlyContinue | ||
| if ($pkg) {{ | ||
| $pkg | Select-Object Name, Id, Version, Publisher, Description, Homepage, License, LicenseUrl, PrivacyUrl, Copyright, ReleaseNotes, Tags | ConvertTo-Json -Depth 2 | ||
| }} | ||
| """ |
Contributor
There was a problem hiding this comment.
Command injection vulnerability via unsanitized package_id.
Same issue as in _search_via_powershell: the package_id is interpolated directly into the PowerShell command without sanitization.
🔒 Proposed fix
+ # Sanitize package_id to prevent command injection
+ safe_id = package_id.replace("'", "''").replace("`", "``")
ps_script = f"""
Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue
- $pkg = Get-WinGetPackage -Id '{package_id}' -ErrorAction SilentlyContinue
+ $pkg = Get-WinGetPackage -Id '{safe_id}' -ErrorAction SilentlyContinue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ps_script = f""" | |
| Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue | |
| $pkg = Get-WinGetPackage -Id '{package_id}' -ErrorAction SilentlyContinue | |
| if ($pkg) {{ | |
| $pkg | Select-Object Name, Id, Version, Publisher, Description, Homepage, License, LicenseUrl, PrivacyUrl, Copyright, ReleaseNotes, Tags | ConvertTo-Json -Depth 2 | |
| }} | |
| """ | |
| # Sanitize package_id to prevent command injection | |
| safe_id = package_id.replace("'", "''").replace("`", "``") | |
| ps_script = f""" | |
| Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue | |
| $pkg = Get-WinGetPackage -Id '{safe_id}' -ErrorAction SilentlyContinue | |
| if ($pkg) {{ | |
| $pkg | Select-Object Name, Id, Version, Publisher, Description, Homepage, License, LicenseUrl, PrivacyUrl, Copyright, ReleaseNotes, Tags | ConvertTo-Json -Depth 2 | |
| }} | |
| """ |
🤖 Prompt for AI Agents
In `@src/switchcraft_winget/utils/winget.py` around lines 582 - 588, The
PowerShell snippet in ps_script interpolates package_id directly (vulnerable to
command injection), so change the script to accept a parameter (e.g.,
param($id)) and reference $id inside the script instead of inserting package_id
into the string, and invoke the PowerShell process with an ArgumentList (or
equivalent API) to pass package_id as a separate argument; do the same pattern
used in _search_via_powershell (or adapt that secure approach) so package_id is
not concatenated into ps_script.
Comment on lines
+637
to
+646
| ps_script = f""" | ||
| Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue | ||
| $result = Install-WinGetPackage -Id '{package_id}' -Scope {scope_param} -AcceptPackageAgreements -AcceptSourceAgreements -ErrorAction Stop | ||
| if ($result -and $result.ExitCode -eq 0) {{ | ||
| Write-Output "SUCCESS" | ||
| }} else {{ | ||
| Write-Output "FAILED" | ||
| exit 1 | ||
| }} | ||
| """ |
Contributor
There was a problem hiding this comment.
Command injection vulnerability in install path.
The package_id parameter is also unsanitized here. Apply the same sanitization fix.
🔒 Proposed fix
scope_param = "Machine" if scope == "machine" else "User"
+ safe_id = package_id.replace("'", "''").replace("`", "``")
ps_script = f"""
Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue
- $result = Install-WinGetPackage -Id '{package_id}' -Scope {scope_param} -AcceptPackageAgreements -AcceptSourceAgreements -ErrorAction Stop
+ $result = Install-WinGetPackage -Id '{safe_id}' -Scope {scope_param} -AcceptPackageAgreements -AcceptSourceAgreements -ErrorAction Stop🤖 Prompt for AI Agents
In `@src/switchcraft_winget/utils/winget.py` around lines 637 - 646, The PS script
builds a command using the unsanitized package_id (ps_script /
Install-WinGetPackage) leading to command injection; sanitize or validate
package_id the same way you handled other inputs (e.g., escape/whitelist or use
a safe helper function used elsewhere) before interpolating it into ps_script,
and ensure the sanitized value is used in the Install-WinGetPackage -Id
parameter and any related string construction.
Comment on lines
+664
to
+702
| def _download_via_powershell(self, package_id: str, dest_dir: Path) -> Optional[Path]: | ||
| """Download a package using PowerShell (via Get-WinGetPackage and manual download).""" | ||
| try: | ||
| # PowerShell module doesn't have a direct download cmdlet, so we fall back to CLI | ||
| # But we can use it to verify the package exists first | ||
| if not self._ensure_winget_module(): | ||
| return None | ||
|
|
||
| ps_script = f""" | ||
| Import-Module Microsoft.WinGet.Client -ErrorAction SilentlyContinue | ||
| $pkg = Get-WinGetPackage -Id '{package_id}' -ErrorAction SilentlyContinue | ||
| if ($pkg) {{ | ||
| Write-Output "EXISTS" | ||
| }} else {{ | ||
| Write-Output "NOT_FOUND" | ||
| exit 1 | ||
| }} | ||
| """ | ||
| cmd = ["powershell", "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", "-Command", ps_script] | ||
| startupinfo = self._get_startup_info() | ||
| kwargs = {} | ||
| if startupinfo: | ||
| kwargs['startupinfo'] = startupinfo | ||
| import sys | ||
| if sys.platform == "win32": | ||
| if hasattr(subprocess, 'CREATE_NO_WINDOW'): | ||
| kwargs['creationflags'] = subprocess.CREATE_NO_WINDOW | ||
| else: | ||
| kwargs['creationflags'] = 0x08000000 | ||
| proc = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", errors="ignore", timeout=30, **kwargs) | ||
|
|
||
| # If package exists, use CLI to download (PowerShell module doesn't have download cmdlet) | ||
| if proc.returncode == 0 and "EXISTS" in proc.stdout: | ||
| # Fall through to CLI download | ||
| return None | ||
| return None | ||
| except Exception as e: | ||
| logger.debug(f"PowerShell download check error: {e}") | ||
| return None |
Contributor
There was a problem hiding this comment.
Function name is misleading; always returns None.
_download_via_powershell doesn't actually download anything—it only verifies the package exists, then returns None in all cases (lines 696-699). The CLI fallback in download_package handles the actual download regardless.
Consider either:
- Renaming to
_verify_package_exists_via_powershell - Removing this function entirely since the existence check adds a round-trip without meaningful benefit
Also, line 674 has the same command injection vulnerability with package_id.
💡 Suggested simplification
If the verification doesn't prevent CLI download attempts, consider removing this pre-check:
def download_package(self, package_id: str, dest_dir: Path) -> Optional[Path]:
"""Download a package installer to dest_dir using Winget CLI."""
- # Try PowerShell first (preferred method)
- try:
- result = self._download_via_powershell(package_id, dest_dir)
- if result:
- return result
- except Exception as e:
- logger.debug(f"PowerShell download failed for {package_id}, falling back to CLI: {e}")
-
- # Fallback to CLI
cmd = ["winget", "download", "--id", package_id, ...🤖 Prompt for AI Agents
In `@src/switchcraft_winget/utils/winget.py` around lines 664 - 702, The
_download_via_powershell function is misleading because it never downloads and
always returns None; either remove the function or rename it (e.g.,
_verify_package_exists_via_powershell) and adjust callers (like
download_package) accordingly so behavior is clear; if you keep the existence
check, ensure it actually affects control flow (return a boolean/raise on not
found) and avoid command injection by not interpolating package_id into the
PowerShell string—pass it as a safe argument or validate/escape it before use,
and update any logic that expects a Path (e.g., callers of
_download_via_powershell) to handle the new return type.
Comment on lines
372
to
+384
| logger.info(f"Fetching package details for: {short_info['Id']}") | ||
| full = self.winget.get_package_details(short_info['Id']) | ||
| logger.debug(f"Raw package details received: {list(full.keys()) if full else 'empty'}") | ||
| merged = {**short_info, **full} | ||
| self.current_pkg = merged | ||
| logger.info(f"Package details fetched, showing UI for: {merged.get('Name', 'Unknown')}") | ||
| logger.info(f"Merged package data keys: {list(merged.keys())}") | ||
|
|
||
| # Validate that we got some data | ||
| if not full: | ||
| logger.warning(f"get_package_details returned empty dict for {short_info['Id']}") | ||
| raise Exception(f"No details found for package: {short_info['Id']}") | ||
|
|
Contributor
There was a problem hiding this comment.
Validate details before merging to avoid a TypeError
full can be None; {**short_info, **full} will raise before your intended “No details found” error. Validate full (or coerce to {}) before the merge.
🐛 Proposed fix
- full = self.winget.get_package_details(short_info['Id'])
- logger.debug(f"Raw package details received: {list(full.keys()) if full else 'empty'}")
- merged = {**short_info, **full}
- self.current_pkg = merged
+ full = self.winget.get_package_details(short_info['Id'])
+ logger.debug(f"Raw package details received: {list(full.keys()) if full else 'empty'}")
+ if not full:
+ logger.warning(f"get_package_details returned empty dict for {short_info['Id']}")
+ raise Exception(f"No details found for package: {short_info['Id']}")
+ merged = {**short_info, **full}
+ self.current_pkg = merged🤖 Prompt for AI Agents
In `@src/switchcraft/gui_modern/views/winget_view.py` around lines 372 - 384, The
code merges short_info and full without checking full, which can be None and
cause a TypeError; update the block after calling
self.winget.get_package_details(short_info['Id']) (the get_package_details call)
to validate/coerce full (e.g., if full is None: log a warning and set full = {}
or raise immediately) before running merged = {**short_info, **full}, then set
self.current_pkg = merged and keep the existing logger messages; ensure the
existing check that raises on empty details still runs after the validation or
replace it with the immediate None-handling logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.