Replace MD5 with SHA256 in security-sensitive operations and fix CodeQL analysis#18
Conversation
Co-authored-by: sphildreth <193334+sphildreth@users.noreply.github.com>
Co-authored-by: sphildreth <193334+sphildreth@users.noreply.github.com>
… warnings Co-authored-by: sphildreth <193334+sphildreth@users.noreply.github.com>
|
@copilot Resolve why CodeQL (javascript-typescript) is failing on this PR |
…red libraries Co-authored-by: sphildreth <193334+sphildreth@users.noreply.github.com>
Fixed in commit 9f26a67. The CodeQL javascript-typescript analysis was failing because it was attempting to analyze the entire repository, including:
Updated |
There was a problem hiding this comment.
Pull request overview
This PR addresses 21 security concerns identified by code scanning, primarily focusing on replacing weak MD5 cryptographic hashing with SHA256 in security-sensitive operations while preserving MD5 usage where mandated by external API specifications.
Key Changes:
- Migrated internal security operations (session cookies, ETags, file duplicate detection) from MD5 to SHA256
- Replaced
new Random()withRandom.Sharedfor thread-safe randomization - Added comprehensive documentation for unavoidable MD5 usage in OpenSubsonic and Last.fm APIs
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
HashHelper.cs |
Added SHA256 methods and streaming I/O for file hashing; documented MD5 retention for API compatibility |
MelodeeBlazorCookieMiddleware.cs |
Replaced MD5 with SHA256 for session cookie generation and validation |
ControllerBase.cs |
Updated cookie hash verification to use SHA256 |
FileSystemDirectoryInfoExtensions.cs |
Migrated file duplicate detection from MD5 to SHA256 |
OpenSubsonicApiService.cs |
Updated ETag generation to SHA256; replaced new Random() with Random.Shared |
UserService.cs |
Documented MD5 requirement for OpenSubsonic API authentication |
ScrobbleController.cs |
Documented MD5 requirement for Last.fm API authentication |
JellyfinXmlDeserializer.cs |
Documented built-in XXE protection in .NET Core |
ShellHelper.cs |
Enhanced security warnings about shell injection limitations |
SECURITY-FIXES.md |
Comprehensive security audit documentation |
codeql-config.yml |
Refined CodeQL analysis scope to exclude documentation and vendored code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using (var sha256 = SHA256.Create()) | ||
| { | ||
| var data = sha256.ComputeHash(bytes); | ||
|
|
||
| // Create a new StringBuilder to collect the bytes and create a string. | ||
| var sBuilder = new StringBuilder(); | ||
|
|
||
| // Loop through each byte of the hashed data and format each one as a hexadecimal string. | ||
| foreach (var t in data) | ||
| { | ||
| sBuilder.Append(t.ToString("x2")); | ||
| } | ||
|
|
||
| // Return the hexadecimal string. | ||
| return sBuilder.ToString(); | ||
| } |
There was a problem hiding this comment.
Inconsistent resource disposal pattern. The CreateSha256(byte[]? bytes) method uses a using statement block, while CreateSha256(FileInfo file) uses using declarations. For consistency and readability, both methods should use the same pattern. Consider using using declarations (without braces) in the byte array overload to match the FileInfo overload.
| using (var sha256 = SHA256.Create()) | |
| { | |
| var data = sha256.ComputeHash(bytes); | |
| // Create a new StringBuilder to collect the bytes and create a string. | |
| var sBuilder = new StringBuilder(); | |
| // Loop through each byte of the hashed data and format each one as a hexadecimal string. | |
| foreach (var t in data) | |
| { | |
| sBuilder.Append(t.ToString("x2")); | |
| } | |
| // Return the hexadecimal string. | |
| return sBuilder.ToString(); | |
| } | |
| using var sha256 = SHA256.Create(); | |
| var data = sha256.ComputeHash(bytes); | |
| // Create a new StringBuilder to collect the bytes and create a string. | |
| var sBuilder = new StringBuilder(); | |
| // Loop through each byte of the hashed data and format each one as a hexadecimal string. | |
| foreach (var t in data) | |
| { | |
| sBuilder.Append(t.ToString("x2")); | |
| } | |
| // Return the hexadecimal string. | |
| return sBuilder.ToString(); |
| using var md5 = MD5.Create(); | ||
| using var stream = new FileStream(file.FullName, FileMode.Open, FileAccess.Read, FileShare.Read); | ||
| var hash = md5.ComputeHash(stream); | ||
|
|
||
| var sBuilder = new StringBuilder(); | ||
| foreach (var t in hash) | ||
| { | ||
| sBuilder.Append(t.ToString("x2")); | ||
| } | ||
| return sBuilder.ToString(); |
There was a problem hiding this comment.
The hash-to-hex conversion logic is duplicated across multiple methods (CreateMd5(FileInfo), CreateSha256(FileInfo), and CreateSha256(byte[])). Consider extracting this into a private helper method like ConvertHashToHexString(byte[] hash) to reduce duplication and improve maintainability.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Code scanning identified 21 security concerns, primarily weak cryptographic hash usage (MD5) in security-sensitive contexts and weak random number generation.
Changes
Cryptographic Hash Migration
MelodeeBlazorCookieMiddleware.csControllers/OpenSubsonic/ControllerBase.csFileSystemDirectoryInfoExtensions.csOpenSubsonicApiService.cs(4 locations)Random Number Generation
new Random()withRandom.SharedinOpenSubsonicApiService.csfor thread-safe song shufflingAPI-Mandated MD5 (Unchanged)
Two MD5 usages remain by external API requirement:
MD5(password + salt)per specBoth documented with inline comments explaining the external constraint.
Security Hardening
CodeQL Configuration Fix
.github/codeql/codeql-config.ymlto exclude:This focuses CodeQL analysis on actual application JavaScript in the src directory, preventing parsing errors from non-application code.
Example
See
SECURITY-FIXES.mdfor detailed security audit results.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.