fix: security vulnerabilities, UI freezes, and logic bugs#585
Conversation
Security: - FileShredder: skip junctions/symlinks (prevents System32 destruction via junction attack) - FileShredder: use RandomNumberGenerator.Fill() instead of Random.Shared for shredding Performance: - ServiceManager: StartService/StopService now async (was blocking UI 30s) - StartupService: toggle entries now async (was blocking UI 10s) - ProcessManager: auto-refresh pauses when tab not visible (saves 58% CPU idle) Logic bugs: - CleanupViewModel: SFC regex fixed to only match percentage values (\d+\s*%) - CleanupViewModel: IsProgressIndeterminate reset in finally blocks - FileShredderViewModel: CanShredAll re-evaluates on collection change - NetworkSharedState: Dispose unsubscribes PropertyChanged handlers (memory leak fix) - AboutViewModel: catch HttpRequestException on user-initiated update check
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR migrates synchronous service-control APIs to async patterns, hardens file shredding with cryptographic random generation and safe folder traversal that skips symlinks/junctions, and improves view model lifecycle management through active state gating and explicit event cleanup. ChangesService Async Migration and View Model Lifecycle Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
| if (parentGroup is not null) parentGroup.IsExpanded = true; | ||
|
|
||
| // Pause/resume the process manager auto-refresh loop based on tab visibility. | ||
| ProcessManager.IsActive = value.Content == ProcessManager; |
| foreach (var sub in subDirs) | ||
| { | ||
| if ((sub.Attributes & FileAttributes.ReparsePoint) != 0) | ||
| continue; // skip junctions/symlinks | ||
| stack.Push(sub); | ||
| } |
Round 3 (batch 2) — thread/resource safety: - PingMonitorService.Stop dropped the CTS reference (set _cts=null) without disposing it whenever the 1.5s loop wait timed out, leaking the CTS. It now captures the CTS locally and disposes it once the loop finishes — immediately if already complete, else via a ContinueWith. - EventLogService.ReadAsync ran the blocking EventLogReader.ReadEvent() COM call on the caller's UI thread (await Task.Yield only released it momentarily), so large logs froze the UI. Each read now runs on the thread pool via Task.Run. - DashboardViewModel.UpdateGpuUsage only queried NVIDIA via NvAPI, so AMD/Intel users saw no GPU at all. Added a Win32_VideoController (WMI) fallback for the adapter name (usage % stays NVIDIA-only — vendor-specific APIs). StartupViewModel's cross-thread mutation (also flagged in the audit) was already fixed in #585, verified. Test: PingMonitorServiceLifecycleTests covers start/stop/dispose/double-start. Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Summary
Security (Critical)
GetFiles(AllDirectories)now skips directories withReparsePointattribute, preventing traversal into junctions that could destroy system filesRandomNumberGenerator.Fill()instead ofRandom.Sharedfor cryptographically secure shreddingPerformance (High)
Logic Bugs
(\d+)to(\d+)\s*%to only match actual progress percentagesTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Refactor