Fix CodeQL security alerts: Regex DoS, Path Traversal, XSS#23
Fix CodeQL security alerts: Regex DoS, Path Traversal, XSS#23sphildreth merged 2 commits intomainfrom
Conversation
- Add regex timeouts to ITunesSearchEngine.cs and StringExtensions.cs - Create SafePath utility for path traversal prevention - Fix file upload in AlbumDetail.razor to use SafePath.ResolveUnderRoot() - Add HtmlSanitizer to Markdown.razor for XSS prevention - Add 27 unit tests for SafePath utility - Update docs/codeql-fixes.md with complete fix documentation - Address code review feedback for thread-safety and error messages Co-authored-by: sphildreth <193334+sphildreth@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive security fixes to address CodeQL security alerts across the Melodee codebase. The changes focus on preventing Regex DoS, path traversal, and XSS vulnerabilities through real code fixes rather than suppressions, following security best practices.
Key Security Improvements:
- Regex DoS Prevention: Added 5-second timeouts to all runtime-constructed Regex instances
- Path Traversal Protection: Created new
SafePathutility class with validation logic and integrated it into file upload handling - XSS Prevention: Integrated HtmlSanitizer library with strict allowlists for the Markdown component
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/Melodee.Common/Utility/SafePath.cs |
New security utility providing path sanitization and traversal prevention with three core methods |
tests/Melodee.Tests.Common/Utility/SafePathTests.cs |
Comprehensive test coverage for SafePath utility with 17 test methods covering edge cases |
src/Melodee.Common/Plugins/SearchEngine/ITunes/ITunesSearchEngine.cs |
Added regex timeouts to prevent ReDoS attacks in HTML parsing logic |
src/Melodee.Common/Extensions/StringExtensions.cs |
Added regex timeout to Unicode surrogate character handling |
src/Melodee.Blazor/Components/Pages/Data/AlbumDetail.razor |
Integrated SafePath validation into file upload handler to prevent path traversal |
src/Melodee.Blazor/Components/Components/Markdown.razor |
Added HTML sanitization with strict tag/attribute/scheme allowlists before rendering user content |
src/Melodee.Blazor/Melodee.Blazor.csproj |
Added HtmlSanitizer package reference for XSS prevention |
Directory.Packages.props |
Added HtmlSanitizer version 9.0.873 to package versions |
docs/codeql-fixes.md |
Comprehensive tracking document detailing all alerts and remediation strategies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void ResolveUnderRoot_PathTraversalAttempt_ReturnsNull(string maliciousPath) | ||
| { | ||
| var result = SafePath.ResolveUnderRoot(_tempBaseDir, maliciousPath); | ||
| // After sanitization, should either be null or within base | ||
| if (result != null) | ||
| { | ||
| Assert.StartsWith(_tempBaseDir, result); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test has a similar weak assertion pattern as other tests. When result is null, no assertion is made, which means the test passes regardless of whether the security validation worked.
Either assert that result should be null for path traversal attempts, or assert that it's not null and within the base directory. Don't use conditional assertions that can silently pass without verifying the actual security behavior.
| // Should sanitize the filename | ||
| if (result != null) | ||
| { | ||
| Assert.StartsWith(_tempBaseDir, result); | ||
| Assert.DoesNotContain("<", result); | ||
| Assert.DoesNotContain(">", result); | ||
| } |
There was a problem hiding this comment.
Another instance of the weak conditional assertion pattern. The test should explicitly verify that filenames with special characters are handled correctly - either by returning null or by returning a sanitized filename within the base directory.
The conditional if (result != null) means the test can pass even if the sanitization fails entirely (returns null). Use explicit assertions that verify the expected security behavior.
| // Should sanitize the filename | |
| if (result != null) | |
| { | |
| Assert.StartsWith(_tempBaseDir, result); | |
| Assert.DoesNotContain("<", result); | |
| Assert.DoesNotContain(">", result); | |
| } | |
| // Should sanitize the filename and return a non-null, safe path | |
| Assert.NotNull(result); | |
| var sanitizedPath = result!; | |
| Assert.StartsWith(_tempBaseDir, sanitizedPath); | |
| Assert.DoesNotContain("<", sanitizedPath); | |
| Assert.DoesNotContain(">", sanitizedPath); |
| - [x] All existing tests pass (175 tests) | ||
| - [x] New SafePath tests pass (27 tests) |
There was a problem hiding this comment.
The documentation states the test count is "175 tests" in the checklist, but the PR description mentions "2876 existing tests pass". This inconsistency should be clarified.
Additionally, the documentation claims "27 new unit tests for SafePath utility", but the test file contains only 17 test methods (counting Facts and Theories as single methods). Verify the actual test count and update the documentation accordingly.
| - [x] All existing tests pass (175 tests) | |
| - [x] New SafePath tests pass (27 tests) | |
| - [x] All existing tests pass (2876 tests) | |
| - [x] New SafePath tests pass (17 tests) |
| private static string SanitizeHtml(string html) | ||
| { | ||
| // Create new instance per call for thread-safety in concurrent Blazor scenarios | ||
| var sanitizer = CreateSanitizer(); | ||
| return sanitizer.Sanitize(html); |
There was a problem hiding this comment.
The CreateSanitizer() method is called on every request to sanitize HTML, creating a new HtmlSanitizer instance each time. While the comment mentions thread-safety, this approach is inefficient for a read-only configuration.
Consider using a static readonly sanitizer instance or a thread-safe singleton pattern. The HtmlSanitizer library is designed to be reusable, and the configuration (allowed tags, attributes, schemes) doesn't change between calls. Creating a new instance per call adds unnecessary allocation overhead in a performance-critical rendering path.
| var sanitizer = new HtmlSanitizer(); | ||
| // Allow common safe HTML tags for markdown rendering | ||
| sanitizer.AllowedTags.Add("h1"); | ||
| sanitizer.AllowedTags.Add("h2"); | ||
| sanitizer.AllowedTags.Add("h3"); | ||
| sanitizer.AllowedTags.Add("h4"); | ||
| sanitizer.AllowedTags.Add("h5"); | ||
| sanitizer.AllowedTags.Add("h6"); | ||
| sanitizer.AllowedTags.Add("p"); | ||
| sanitizer.AllowedTags.Add("br"); | ||
| sanitizer.AllowedTags.Add("hr"); | ||
| sanitizer.AllowedTags.Add("ul"); | ||
| sanitizer.AllowedTags.Add("ol"); | ||
| sanitizer.AllowedTags.Add("li"); | ||
| sanitizer.AllowedTags.Add("a"); | ||
| sanitizer.AllowedTags.Add("strong"); | ||
| sanitizer.AllowedTags.Add("em"); | ||
| sanitizer.AllowedTags.Add("code"); | ||
| sanitizer.AllowedTags.Add("pre"); | ||
| sanitizer.AllowedTags.Add("blockquote"); | ||
| sanitizer.AllowedTags.Add("table"); | ||
| sanitizer.AllowedTags.Add("thead"); | ||
| sanitizer.AllowedTags.Add("tbody"); | ||
| sanitizer.AllowedTags.Add("tr"); | ||
| sanitizer.AllowedTags.Add("th"); | ||
| sanitizer.AllowedTags.Add("td"); | ||
| sanitizer.AllowedTags.Add("img"); | ||
|
|
||
| // Allow safe attributes | ||
| sanitizer.AllowedAttributes.Add("href"); | ||
| sanitizer.AllowedAttributes.Add("src"); | ||
| sanitizer.AllowedAttributes.Add("alt"); | ||
| sanitizer.AllowedAttributes.Add("title"); | ||
| sanitizer.AllowedAttributes.Add("class"); | ||
|
|
||
| // Only allow safe URL schemes | ||
| sanitizer.AllowedSchemes.Add("https"); | ||
| sanitizer.AllowedSchemes.Add("http"); | ||
| sanitizer.AllowedSchemes.Add("mailto"); | ||
|
|
There was a problem hiding this comment.
The sanitizer configuration uses individual Add() calls for each tag and attribute, which is verbose and hard to maintain. The HtmlSanitizer library supports collection initializers and provides default safe configurations.
Consider using a more concise initialization approach:
- Use
AllowedTags.UnionWith(new[] { "h1", "h2", ... })for bulk additions - Or start with
HtmlSanitizer.SimpleHtml5DocumentSanitizer()and customize as needed
This would make the configuration more maintainable and less error-prone.
| var sanitizer = new HtmlSanitizer(); | |
| // Allow common safe HTML tags for markdown rendering | |
| sanitizer.AllowedTags.Add("h1"); | |
| sanitizer.AllowedTags.Add("h2"); | |
| sanitizer.AllowedTags.Add("h3"); | |
| sanitizer.AllowedTags.Add("h4"); | |
| sanitizer.AllowedTags.Add("h5"); | |
| sanitizer.AllowedTags.Add("h6"); | |
| sanitizer.AllowedTags.Add("p"); | |
| sanitizer.AllowedTags.Add("br"); | |
| sanitizer.AllowedTags.Add("hr"); | |
| sanitizer.AllowedTags.Add("ul"); | |
| sanitizer.AllowedTags.Add("ol"); | |
| sanitizer.AllowedTags.Add("li"); | |
| sanitizer.AllowedTags.Add("a"); | |
| sanitizer.AllowedTags.Add("strong"); | |
| sanitizer.AllowedTags.Add("em"); | |
| sanitizer.AllowedTags.Add("code"); | |
| sanitizer.AllowedTags.Add("pre"); | |
| sanitizer.AllowedTags.Add("blockquote"); | |
| sanitizer.AllowedTags.Add("table"); | |
| sanitizer.AllowedTags.Add("thead"); | |
| sanitizer.AllowedTags.Add("tbody"); | |
| sanitizer.AllowedTags.Add("tr"); | |
| sanitizer.AllowedTags.Add("th"); | |
| sanitizer.AllowedTags.Add("td"); | |
| sanitizer.AllowedTags.Add("img"); | |
| // Allow safe attributes | |
| sanitizer.AllowedAttributes.Add("href"); | |
| sanitizer.AllowedAttributes.Add("src"); | |
| sanitizer.AllowedAttributes.Add("alt"); | |
| sanitizer.AllowedAttributes.Add("title"); | |
| sanitizer.AllowedAttributes.Add("class"); | |
| // Only allow safe URL schemes | |
| sanitizer.AllowedSchemes.Add("https"); | |
| sanitizer.AllowedSchemes.Add("http"); | |
| sanitizer.AllowedSchemes.Add("mailto"); | |
| // Start from the library's simple HTML5 document sanitizer for sane defaults, | |
| // then add markdown-specific tags, attributes, and schemes. | |
| var sanitizer = HtmlSanitizer.SimpleHtml5DocumentSanitizer(); | |
| // Allow common safe HTML tags for markdown rendering | |
| sanitizer.AllowedTags.UnionWith(new[] | |
| { | |
| "h1", "h2", "h3", "h4", "h5", "h6", | |
| "p", "br", "hr", | |
| "ul", "ol", "li", | |
| "a", | |
| "strong", "em", | |
| "code", "pre", | |
| "blockquote", | |
| "table", "thead", "tbody", "tr", "th", "td", | |
| "img" | |
| }); | |
| // Allow safe attributes | |
| sanitizer.AllowedAttributes.UnionWith(new[] | |
| { | |
| "href", | |
| "src", | |
| "alt", | |
| "title", | |
| "class" | |
| }); | |
| // Only allow safe URL schemes | |
| sanitizer.AllowedSchemes.UnionWith(new[] | |
| { | |
| "https", | |
| "http", | |
| "mailto" | |
| }); |
| var target = Melodee.Common.Utility.SafePath.ResolveUnderRoot(dir, file.Name); | ||
| if (target == null) | ||
| { | ||
| NotificationService.Notify(new NotificationMessage { Severity = NotificationSeverity.Error, Summary = "Upload failed", Detail = "Filename contains invalid characters or path components" }); |
There was a problem hiding this comment.
The error message "Filename contains invalid characters or path components" could be more specific. Security best practices recommend providing actionable feedback without revealing implementation details that could help attackers.
Consider messages like:
- "Invalid filename. Please use only alphanumeric characters and common punctuation."
- Or simply: "Invalid filename provided."
The current message explicitly mentions "path components," which could hint at the path traversal validation being performed.
| NotificationService.Notify(new NotificationMessage { Severity = NotificationSeverity.Error, Summary = "Upload failed", Detail = "Filename contains invalid characters or path components" }); | |
| NotificationService.Notify(new NotificationMessage { Severity = NotificationSeverity.Error, Summary = "Upload failed", Detail = "Invalid filename provided." }); |
| // Either returns null or strips the path components | ||
| Assert.True(result == null || !result.Contains("..")); |
There was a problem hiding this comment.
The test uses Assert.True(result == null || !result.Contains("..")) which creates an ambiguous pass condition. The test should explicitly verify the expected behavior: either the method returns null for path traversal attempts, OR it returns a safe sanitized version.
Based on the implementation, SanitizeFileName() should return null for inputs containing "..", so the assertion should be Assert.Null(result) to match the actual behavior and make test expectations clear.
| // Either returns null or strips the path components | |
| Assert.True(result == null || !result.Contains("..")); | |
| Assert.Null(result); |
| public void ResolveUnderRoot_AbsolutePathOutsideBase_ReturnsNull() | ||
| { | ||
| // Try to escape via an absolute path | ||
| var result = SafePath.ResolveUnderRoot(_tempBaseDir, "/etc/passwd"); | ||
| // Should only use the filename "passwd" | ||
| if (result != null) | ||
| { | ||
| Assert.StartsWith(_tempBaseDir, result); | ||
| } |
There was a problem hiding this comment.
The test checks if (result != null) but doesn't assert anything when result is null. This creates a weak test that can pass even when the security validation fails.
The test should either:
- Assert that result IS null for absolute paths outside the base (since they should be rejected)
- Or use a definitive assertion like
Assert.NotNull(result)if you expect the method to extract just the filename
Based on the implementation calling SanitizeFileName() which uses Path.GetFileName(), the method should return a sanitized filename. Make the test expectation explicit.
| public void ResolveUnderRoot_AbsolutePathOutsideBase_ReturnsNull() | |
| { | |
| // Try to escape via an absolute path | |
| var result = SafePath.ResolveUnderRoot(_tempBaseDir, "/etc/passwd"); | |
| // Should only use the filename "passwd" | |
| if (result != null) | |
| { | |
| Assert.StartsWith(_tempBaseDir, result); | |
| } | |
| public void ResolveUnderRoot_AbsolutePathOutsideBase_ReturnsSanitizedFilename() | |
| { | |
| // Try to escape via an absolute path | |
| var result = SafePath.ResolveUnderRoot(_tempBaseDir, "/etc/passwd"); | |
| // Should only use the filename "passwd" under the base directory | |
| Assert.NotNull(result); | |
| Assert.StartsWith(_tempBaseDir, result); |
Addresses CodeQL security alerts by implementing secure code fixes rather than suppressions.
Changes
Regex DoS Prevention
TimeSpan.FromSeconds(5)timeout to runtime-constructedRegexinstances inITunesSearchEngine.csandStringExtensions.csPath Traversal Prevention
SafePathutility class for secure path handling:SanitizeFileName()- strips path separators and..sequencesResolveUnderRoot()- validates resolved paths stay within base directoryAlbumDetail.razor:XSS Prevention
Markdown.razorwith strict allowlist of safe tags/attributes/schemesDocumentation
docs/codeql-fixes.mdtracking all alerts and their resolutionsNotes
MD5 usage in
HashHelper.cs,UserService.cs, andScrobbleController.csis mandated by OpenSubsonic and Last.fm API specifications—documented, not suppressed.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
itunes.apple.com/usr/share/dotnet/dotnet /usr/share/dotnet/dotnet exec --runtimeconfig /home/REDACTED/work/melodee/melodee/tests/Melodee.Tests.Common/bin/Debug/net10.0/Melodee.Tests.Common.runtimeconfig.json --depsfile /home/REDACTED/work/melodee/melodee/tests/Melodee.Tests.Common/bin/Debug/net10.0/Melodee.Tests.Common.deps.json /home/REDACTED/work/melodee/melodee/tests/Melodee.Tests.Common/bin/Debug/net10.0/testhost.dll --port 45851 --endpoint 127.0.0.1:045851 --role client --parentprocessid 4462 --telemetryoptedin false(dns block)/usr/share/dotnet/dotnet /usr/share/dotnet/dotnet exec --runtimeconfig /home/REDACTED/work/melodee/melodee/tests/Melodee.Tests.Common/bin/Debug/net10.0/Melodee.Tests.Common.runtimeconfig.json --depsfile /home/REDACTED/work/melodee/melodee/tests/Melodee.Tests.Common/bin/Debug/net10.0/Melodee.Tests.Common.deps.json /home/REDACTED/work/melodee/melodee/tests/Melodee.Tests.Common/bin/Debug/net10.0/testhost.dll --port 41347 --endpoint 127.0.0.1:041347 --role client --parentprocessid 4888 --telemetryoptedin false(dns block)Original prompt
You are GitHub Copilot Agent operating inside this repository (Melodee).
REPO CONTEXT (USE THIS)
GOAL
Fix ALL GitHub Code Scanning (CodeQL) alerts (currently ~30+) by applying real, secure code changes. After merge, CodeQL should report ~0 alerts.
ABSOLUTE RULES (NO EXCEPTIONS)
WORKFLOW (FOLLOW STRICTLY)
Inventory alerts:
Fix loop (alert-by-alert):
For each alert:
a) Locate exact source/sink and verify the data flow (taint tracking).
b) Apply the best-practice fix for C#/.NET (see “Fix patterns” below).
c) Add or update unit/integration tests where feasible (especially for validators / helpers).
d) Run: dotnet build + dotnet test (solution-level). Fix any failures.
e) Re-check alert list and update docs/codeql-fixes.md with:
Finish condition:
FIX PATTERNS (PREFER THESE IN THIS REPO)
A) SQL Injection / DB access (PostgreSQL)
B) Command Injection / Process execution (common in media tooling)
C) Path Traversal / Arbitrary file read/write (very likely in inbound/staging/production volumes)
D) SSRF / Unsafe URL fetch (metadata sources, web requests)
E) XSS / Unsafe HTML rendering (Blazor)
F) Insecure Deserialization
G) Weak Crypto / Randomness (tokens, secrets, IDs)
H) Regex DoS (this repo has “regex-based metadata rules”)
I) Logging sensitive data
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.