Skip to content

fix: 9 security hardening fixes#1

Open
khalidelmerrah wants to merge 1 commit intoilukezippo:mainfrom
khalidelmerrah:security-fixes
Open

fix: 9 security hardening fixes#1
khalidelmerrah wants to merge 1 commit intoilukezippo:mainfrom
khalidelmerrah:security-fixes

Conversation

@khalidelmerrah
Copy link
Copy Markdown

@khalidelmerrah khalidelmerrah commented Apr 16, 2026

Summary

Security review of windows_fixer.py found 9 vulnerabilities ranging from critical (command injection) to low (thread safety). All fixes are in a single file with no new dependencies, no UI changes, and no breaking changes.

Fixes

# Severity Fix
1 Critical Command injection in CHKDSK fix mode - removed cmd /c string interpolation, use list form + drive letter regex validation
2 Critical Argument injection in admin relaunch - replaced manual quoting with subprocess.list2cmdline
3 High Version tag from GitHub API sanitized to only allow v0-9. characters
4 Medium Settings file validated after loading - type enforcement + language whitelist
5 Medium resource_path() rejects .. and absolute path prefixes (path traversal)
6 Low shutil.rmtree uses onerror handler instead of ignore_errors=True - logs failed deletions
7 Low Critical functions use specific exception types instead of bare except: pass
8 Low CommandRunner cancel/skip flags use threading.Event instead of bare booleans
9 Low BUILD_DATE reads from env var with fallback (accurate in CI builds)

Details

Fix 1 (Command Injection): step_chkdsk() previously used ["cmd", "/c", f"chkdsk {drive} /f"] which allows shell injection if the drive string is manipulated. Now uses ["chkdsk", drive, "/f"] with regex validation [A-Z]:.

Fix 2 (Argument Injection): relaunch_as_admin() wrapped args in f'"{a}"' without escaping embedded quotes. Now uses subprocess.list2cmdline() which handles all edge cases.

No new dependencies. No UI changes. No breaking changes.

Test plan

  • Verify app launches normally
  • Test CHKDSK in both scan and fix mode
  • Test "Run as Admin" button
  • Test "Check for Updates"
  • Verify settings save/load works
  • Test cancel and skip during operations

1. Command injection in CHKDSK fix mode - removed cmd /c, use list form + drive regex validation
2. Argument injection in admin relaunch - use subprocess.list2cmdline
3. Version tag sanitization - strip non-version chars from GitHub API response
4. Settings validation - added _sanitize_settings() with type + whitelist checks
5. Path traversal in resource_path - reject .. and absolute prefixes
6. Silent deletion failures - shutil.rmtree onerror handler logs failed items
7. Narrowed exception handling - specific types instead of bare except: pass
8. Thread-safe signals - threading.Event instead of bare booleans for cancel/skip
9. BUILD_DATE from env var - accurate build timestamps in CI

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant