fix: dynamic user-path sanitization in LogService and cache FontFamily in MarkdownTextBlock#444
Conversation
📝 WalkthroughWalkthroughThis PR adds two fixes: LogService now derives its user path regex at startup from the actual user profile parent directory (with a fallback), and MarkdownTextBlock caches the Consolas FontFamily to avoid per-render allocations. ChangesPerformance and Path Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
…y in MarkdownTextBlock
61c5032 to
675b14c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SysManager/SysManager/Services/LogService.cs`:
- Around line 24-38: BuildUserPathRegex currently returns a regex that only
matches the discovered userProfile directory (which can be a system profile) and
so misses normal C:\Users\username paths; change BuildUserPathRegex to build and
return a single Regex that alternates the escaped usersDir-based pattern and the
fallback drive-based pattern (e.g.
"(?:<escapedUsersDir>[^\\]+|[A-Z]:\\Users\\[^\\]+)") with RegexOptions.Compiled
and case-insensitive; ensure you escape the usersDir + "\" correctly before
inserting into the alternation so both patterns are matched by the single
returned Regex used elsewhere (function BuildUserPathRegex).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 971e988f-b444-4f4e-8f86-b9b52b7a3590
📥 Commits
Reviewing files that changed from the base of the PR and between 61c5032b5fce8dfcf96a6218b9a56eeb451568cb and 675b14c.
📒 Files selected for processing (3)
CHANGELOG.mdSysManager/SysManager/Helpers/MarkdownTextBlock.csSysManager/SysManager/Services/LogService.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
CHANGELOG.md (1)
17-27: LGTM!SysManager/SysManager/Helpers/MarkdownTextBlock.cs (1)
88-90: LGTM!Also applies to: 117-117
SysManager/SysManager/Services/LogService.cs (1)
19-23: LGTM!
| private static Regex BuildUserPathRegex() | ||
| { | ||
| var userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); | ||
| if (!string.IsNullOrEmpty(userProfile)) | ||
| { | ||
| var usersDir = Path.GetDirectoryName(userProfile); | ||
| if (!string.IsNullOrEmpty(usersDir)) | ||
| { | ||
| var escaped = Regex.Escape(usersDir + @"\"); | ||
| return new Regex($@"(?i)({escaped})[^\\]+", RegexOptions.Compiled); | ||
| } | ||
| } | ||
| // Fallback: match any drive letter followed by \Users\<username> | ||
| return new Regex(@"(?i)([A-Z]:\\Users\\)[^\\]+", RegexOptions.Compiled); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET on Windows, what exact values can Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) return for LocalSystem/NetworkService accounts, and can it be non-empty while not pointing to a standard multi-user root like X:\Users\ ?
💡 Result:
In.NET on Windows, Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) returns the “user profile” path for whatever security context the current process is running under, and it can be empty if the corresponding folder does not physically exist. 1) Exact non-empty values for LocalSystem - For the LocalSystem service account, the returned UserProfile is typically: C:\Windows\System32\Config\SystemProfile This matches reports from.NET code running in a service context where Environment.GetFolderPath(UserProfile) yields C:\Windows\system32\config\systemprofile (and similarly used when building AppData paths). [1][2] 2) NetworkService / LocalService - The.NET/environment APIs compute the UserProfile path based on the running account’s profile. Microsoft’s LocalSystem documentation explains that LocalSystem is a special service account used by the SCM and notes the token/SID context, and it contrasts LocalSystem with LocalService/NetworkService as alternatives for services. [3] - However, in the sources retrieved here, I did not find a similarly explicit, authoritative “exact string” mapping for NetworkService specifically (e.g., the exact folder path string) comparable to the LocalSystem/SystemProfile mapping above. 3) Can it be non-empty while not pointing to a standard multi-user root like X:\Users? - Yes, at least for LocalSystem: the returned path is not under X:\Users\…; it is under the system profile directory (C:\Windows\System32\Config\SystemProfile). [1][2] - Empty-string behavior: Environment.GetFolderPath returns an empty string (“”) if the requested special-folder path does not physically exist on the computer. [4] Bottom line - LocalSystem: non-empty UserProfile path is expected to be C:\Windows\System32\Config\SystemProfile, i.e., non-X:\Users and therefore can meet your “non-empty while not pointing to a standard multi-user root” condition. [1][2] - NetworkService: non-empty is possible, but I cannot, from the sourced material gathered here, state the exact value it will return for NetworkService. If you tell me your exact OS version (e.g., Windows Server 2008 R2 vs newer) and whether the service is running under LocalSystem vs NetworkService vs a custom domain/service account, I can try one more targeted lookup to pin down the exact NetworkService UserProfile string for Windows Server +.NET version in your scenario.
Citations:
- 1: https://serverfault.com/questions/112927/where-does-the-local-system-account-store-its-personal-files
- 2: https://stackoverflow.com/questions/59599473/get-current-user-directory-from-windows-service-running-under-local-system-accou
- 3: https://learn.microsoft.com/en-us/windows/win32/services/localsystem-account
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.environment.getfolderpath?view=net-8.0
🏁 Script executed:
cat -n SysManager/SysManager/Services/LogService.cs | head -100Repository: laurentiu021/SystemManager
Length of output: 3009
🏁 Script executed:
rg "UserPathRegex|BuildUserPathRegex" SysManager/SysManager/Services/LogService.cs -A 3 -B 2Repository: laurentiu021/SystemManager
Length of output: 593
🏁 Script executed:
rg "Regex\.Replace" SysManager/ -A 1 -B 1 -t csRepository: laurentiu021/SystemManager
Length of output: 708
🌐 Web query:
.NET Regex.Replace named capture group replacement syntax ${groupname} C# documentation
💡 Result:
In.NET, the syntax to reference a named capture group within a replacement string is ${name} [1][2]. When using the Regex.Replace method, you can include the substring matched by a named capturing group (defined in your pattern as (?...)) by placing the group name inside the ${} substitution element [1][2]. Key behaviors of this syntax include: - If the name corresponds to a valid named capturing group defined in the regular expression,.NET substitutes the last substring matched by that group [1][2]. - If the name consists only of digits and does not match a named group,.NET interprets it as a numbered backreference (e.g., ${1}) [1][2]. - If the name is neither a valid named group nor a valid numbered group, it is treated as a literal string [1][2]. Example usage: string input = "First Second"; string pattern = @"(?First) (?Second)"; string replacement = "${second} ${first}"; string result = Regex.Replace(input, pattern, replacement); // Result: "Second First" [3]
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/standard/base-types/substitutions-in-regular-expressions
- 2: https://github.com/dotnet/docs/blob/main/docs/standard/base-types/substitutions-in-regular-expressions.md
- 3: https://stackoverflow.com/questions/12962114/how-to-use-named-groups-when-performing-a-regex-replace
Fallback pattern is bypassed when UserProfile is non-empty but non-standard—user paths can leak in logs.
When the service runs under LocalSystem (or other non-interactive accounts), Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) returns a non-empty path like C:\Windows\System32\Config\SystemProfile. This causes an early return on line 33 with a pattern that only matches paths under that system directory, never reaching the fallback on line 37. If logs contain paths like C:\Users\username\..., they won't be redacted.
Combine both patterns using alternation and return a single regex:
Suggested fix
private static Regex BuildUserPathRegex()
{
var userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile);
+ string pattern = @"(?i)(?<prefix>[A-Z]:\\Users\\)[^\\]+";
+
if (!string.IsNullOrEmpty(userProfile))
{
var usersDir = Path.GetDirectoryName(userProfile);
if (!string.IsNullOrEmpty(usersDir))
{
var escaped = Regex.Escape(usersDir + @"\");
- return new Regex($@"(?i)({escaped})[^\\]+", RegexOptions.Compiled);
+ pattern = $@"(?i)(?<prefix>{escaped}|[A-Z]:\\Users\\)[^\\]+";
}
}
- // Fallback: match any drive letter followed by \Users\<username>
- return new Regex(@"(?i)([A-Z]:\\Users\\)[^\\]+", RegexOptions.Compiled);
+
+ return new Regex(pattern, RegexOptions.Compiled);
}Also update line 68:
- return UserPathRegex.Replace(path, "$1[user]");
+ return UserPathRegex.Replace(path, "${prefix}[user]");📝 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.
| private static Regex BuildUserPathRegex() | |
| { | |
| var userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); | |
| if (!string.IsNullOrEmpty(userProfile)) | |
| { | |
| var usersDir = Path.GetDirectoryName(userProfile); | |
| if (!string.IsNullOrEmpty(usersDir)) | |
| { | |
| var escaped = Regex.Escape(usersDir + @"\"); | |
| return new Regex($@"(?i)({escaped})[^\\]+", RegexOptions.Compiled); | |
| } | |
| } | |
| // Fallback: match any drive letter followed by \Users\<username> | |
| return new Regex(@"(?i)([A-Z]:\\Users\\)[^\\]+", RegexOptions.Compiled); | |
| } | |
| private static Regex BuildUserPathRegex() | |
| { | |
| var userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); | |
| string pattern = @"(?i)(?<prefix>[A-Z]:\\Users\\)[^\\]+"; | |
| if (!string.IsNullOrEmpty(userProfile)) | |
| { | |
| var usersDir = Path.GetDirectoryName(userProfile); | |
| if (!string.IsNullOrEmpty(usersDir)) | |
| { | |
| var escaped = Regex.Escape(usersDir + @"\"); | |
| pattern = $@"(?i)(?<prefix>{escaped}|[A-Z]:\\Users\\)[^\\]+"; | |
| } | |
| } | |
| return new Regex(pattern, RegexOptions.Compiled); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/Services/LogService.cs` around lines 24 - 38,
BuildUserPathRegex currently returns a regex that only matches the discovered
userProfile directory (which can be a system profile) and so misses normal
C:\Users\username paths; change BuildUserPathRegex to build and return a single
Regex that alternates the escaped usersDir-based pattern and the fallback
drive-based pattern (e.g. "(?:<escapedUsersDir>[^\\]+|[A-Z]:\\Users\\[^\\]+)")
with RegexOptions.Compiled and case-insensitive; ensure you escape the usersDir
+ "\" correctly before inserting into the alternation so both patterns are
matched by the single returned Regex used elsewhere (function
BuildUserPathRegex).
…#450) ## What does this PR do? Fixes two user-reported crashes: 1. **#443 — Encoding 437 crash during chkdsk scan**: On systems where OEM code page 437 is not registered, `Encoding.GetEncoding()` throws `NotSupportedException`. Now wrapped in try-catch with UTF-8 fallback. 2. **#444 — Invalid packageId during uninstall**: Winget package IDs can contain spaces (e.g. `Riot Games.League of Legends`). The validation regex now allows `\s` characters. ## Files changed - `SystemHealthViewModel.cs` — encoding fallback - `UninstallerService.cs` — regex pattern update - `CHANGELOG.md` — v0.28.27 entry Closes #443 Closes #444 ## Type of change - [x] Bug fix (`fix:`) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
…hes (#452) ## What does this PR do? Fixes all issues found during the full code audit: ### Bug fixes (crash-level) - **CleanupViewModel** — SFC and DISM encoding crash (same as #443, missed callers) - **WingetService** — regex too restrictive for package IDs with spaces (same as #444, missed caller) ### CodeQL fixes - Replaced bare `catch { }` with specific exception types in: - DiskHealthService (ManagementException, UnauthorizedAccessException) - FixedDriveService (ManagementException) - MemoryTestService (EventLogException, UnauthorizedAccessException, InvalidOperationException, Win32Exception) - SystemInfoService (ManagementException) - AdminHelper (InvalidOperationException, Win32Exception) ### Documentation - SECURITY.md: version table updated from 0.5.x to 0.28.x - ARCHITECTURE.md: removed stale tab counts ## Type of change - [x] Bug fix (`fix:`) - [x] Code quality / CodeQL (`fix:`) - [x] Documentation (`docs:`) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
…y in MarkdownTextBlock (#444) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
1 participant
Summary
Fix security finding SEC-004 and performance finding PERF-007 from the full code review.
Changes
LogService.cs (SEC-004)
from
Environment.GetFolderPath(UserProfile)instead of assuming a hardcoded<drive>:\Users\pattern.[A-Z]:\\Users\\regex if the environment variableis unavailable (e.g., running as SYSTEM without a user profile).
on a different drive or at a custom path.
MarkdownTextBlock.cs (PERF-007)
FontFamily("Consolas")as aprivate static readonlyfield(
CodeFontFamily) instead of allocating a new instance on every code span render.Previously fixed (verified, no action needed)
Testing
Summary by CodeRabbit
Bug Fixes
Performance