Potential fix for code scanning alert no. 184: Log entries created from user input

[sphildreth]

Potential fix for https://github.com/melodee-project/melodee/security/code-scanning/184

In general, to fix log-forging issues, sanitize user-provided values before logging by removing or neutralizing characters that can break log structure (e.g., \r, \n) or that might be interpreted specially in the log rendering context. For plain text logs, stripping or replacing newline characters is usually sufficient; the message template itself already clearly marks user input as a field.

For this specific code, we should avoid logging request.Username directly and instead log a sanitized version. A simple, non-invasive fix is to normalize/strip newlines from the username before passing it to the logger. To avoid repeating logic, define a small private helper method inside UsersController (since we are allowed to add methods within the snippet’s file) that takes a string and returns a sanitized version with \r and \n removed. Then, use this helper in all three log statements that log the username:

• Line 55–56 (missing credentials case) already uses request.Username ?? "[empty]"; we should wrap that with the sanitizer.
• Line 63–64 (invalid credentials) logs request.Username directly; change it to the sanitized version.
• Line 71–72 (user locked) is the one CodeQL highlighted; change it to use the sanitized username as well.

We don’t need new external dependencies; a simple Replace chain is enough. The helper can be placed near the bottom of the controller class. No changes to imports are required.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

[Copilot]

Pull request overview

This PR addresses a security vulnerability (CodeQL alert #184) by implementing sanitization for user-provided input before logging. The fix prevents log forging/injection attacks where malicious users could inject newline characters into usernames to manipulate log entries, potentially hiding malicious activity or creating fake log entries.

Key Changes:

• Added SanitizeForLog helper method to remove \r and \n characters from strings before logging
• Applied sanitization to all three authentication failure log statements that record usernames
• No external dependencies required; uses simple string replacement

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The null check in SanitizeForLog could be simplified using modern C# null-coalescing patterns. Additionally, consider sanitizing additional log injection characters beyond just newlines. OWASP recommends also handling tab characters (\t) and other control characters that could be used to manipulate log parsing or display.

Consider updating to handle more control characters or at minimum document why only \r and \n are being sanitized.

Suggested change

	private static string SanitizeForLog(string value)
	{
	if (value is null)
	{
	return string.Empty;
	}
	return value
	.Replace("\r", string.Empty)
	.Replace("\n", string.Empty);
	private static string SanitizeForLog(string? value)
	{
	var safeValue = value ?? string.Empty;
	if (safeValue.Length == 0)
	{
	return string.Empty;
	}
	var builder = new StringBuilder(safeValue.Length);
	foreach (var ch in safeValue)
	{
	if (!char.IsControl(ch))
	{
	builder.Append(ch);
	}
	}
	return builder.ToString();

[Copilot]

The SanitizeForLog method should have XML documentation comments to explain its purpose, especially since it's addressing a security concern. This helps future maintainers understand why this sanitization exists and what attacks it prevents.

Add a summary explaining that this method removes newline characters to prevent log forging/injection attacks.

[Copilot]

The implementation lacks test coverage for the new SanitizeForLog method. Given that this method addresses a security vulnerability (log injection), it's important to have tests that verify:

• Newline characters (\n, \r, \r\n) are properly removed
• Null values are handled correctly
• Normal strings without special characters pass through unchanged
• Edge cases like empty strings work as expected

Add unit tests to ensure this security measure works correctly and doesn't regress in future changes.