Fix #1318: Improve HTTP Error Handling with Sanitized Logging#2285
Fix #1318: Improve HTTP Error Handling with Sanitized Logging#2285algsoch wants to merge 1 commit intoergoplatform:masterfrom
Conversation
…ed logging - Add sanitizeErrorMessage() to filter non-printable characters - Truncate long error messages to prevent log pollution - Handle IllegalRequestException for malformed HTTP requests - Prevent 'HTTP method too long' errors from cluttering logs - Improved ApiErrorHandler with debug logging for bad requests - Enhanced ApiRejectionHandler for better error message handling
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1318 by implementing sanitized error logging for malformed HTTP requests. The implementation adds a sanitizeErrorMessage function to filter non-printable characters and truncate long error messages, preventing log pollution when the Ergo node receives invalid HTTP requests such as HTTPS connections to HTTP endpoints or requests with excessively long methods.
Key changes:
- Added sanitization logic to remove non-printable characters and truncate messages to 200 characters
- Enhanced
IllegalRequestExceptionhandling inApiErrorHandlerwith debug-level logging - Applied sanitization to rejection handlers for malformed content, validation errors, and generic rejections
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
src/main/scala/scorex/core/api/http/ApiErrorHandler.scala |
Added sanitizeErrorMessage function and explicit IllegalRequestException handling with sanitized logging at debug level |
src/main/scala/scorex/core/api/http/ApiRejectionHandler.scala |
Added sanitizeErrorMessage function and applied sanitization to MalformedRequestContentRejection, ValidationRejection, and generic rejection handlers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private def sanitizeErrorMessage(msg: String, maxLength: Int = 200): String = { | ||
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | ||
| if (printable.length > maxLength) { | ||
| printable.take(maxLength) + "... (truncated)" | ||
| } else { | ||
| printable | ||
| } | ||
| } |
There was a problem hiding this comment.
The sanitizeErrorMessage function doesn't handle null input. If rejection message or x.toString returns null (which could happen in edge cases), calling filter on it will throw a NullPointerException. Consider adding a null check at the beginning of the function, such as: if (msg == null) return "null" or Option(msg).getOrElse("").
| private def sanitizeErrorMessage(msg: String, maxLength: Int = 200): String = { | ||
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | ||
| if (printable.length > maxLength) { | ||
| printable.take(maxLength) + "... (truncated)" | ||
| } else { | ||
| printable | ||
| } | ||
| } | ||
|
|
||
| implicit val exceptionHandler: ExceptionHandler = ExceptionHandler { | ||
| case NonFatal(e) => ApiError(e) | ||
| case e: IllegalRequestException => | ||
| // Handle malformed HTTP requests (e.g., "HTTP method too long") with sanitized logging | ||
| val sanitizedMsg = sanitizeErrorMessage(e.getMessage) | ||
| log.debug(s"Illegal request rejected: $sanitizedMsg") | ||
| ApiError.BadRequest(s"Malformed HTTP request: $sanitizedMsg") |
There was a problem hiding this comment.
The new sanitizeErrorMessage function and the handling of IllegalRequestException lack test coverage. Given that the repository has comprehensive HTTP route testing (as seen in src/test/scala/org/ergoplatform/http/routes/), this new functionality should also have tests. Consider adding tests that verify:
- Non-printable characters are filtered out correctly
- Messages longer than 200 characters are truncated with "... (truncated)" suffix
- Null input is handled gracefully
- Newlines, tabs, and carriage returns are preserved
IllegalRequestExceptiontriggers debug logging with sanitized messages
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | ||
| if (printable.length > maxLength) { | ||
| printable.take(maxLength) + "... (truncated)" | ||
| } else { | ||
| printable |
There was a problem hiding this comment.
The sanitization logic preserves newline (\n) and carriage return (\r) characters, which could potentially be used for log injection attacks. An attacker could craft malformed requests with embedded newlines to inject fake log entries. Consider either:
- Replacing newlines and carriage returns with escaped versions (e.g.,
\n→\\n) - Converting them to spaces
- Removing them entirely
This would prevent log injection while still maintaining readability of error messages.
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | |
| if (printable.length > maxLength) { | |
| printable.take(maxLength) + "... (truncated)" | |
| } else { | |
| printable | |
| // Only allow printable ASCII and tab, but not newlines or carriage returns | |
| val filtered = msg.filter(c => (c >= 32 && c < 127) || c == '\t') | |
| // Escape any remaining \n and \r just in case | |
| val escaped = filtered | |
| .replace("\n", "\\n") | |
| .replace("\r", "\\r") | |
| if (escaped.length > maxLength) { | |
| escaped.take(maxLength) + "... (truncated)" | |
| } else { | |
| escaped |
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | ||
| if (printable.length > maxLength) { | ||
| printable.take(maxLength) + "... (truncated)" | ||
| } else { | ||
| printable |
There was a problem hiding this comment.
The sanitization logic preserves newline (\n) and carriage return (\r) characters, which could potentially be used for log injection attacks. An attacker could craft malformed requests with embedded newlines to inject fake log entries. Consider either:
- Replacing newlines and carriage returns with escaped versions (e.g.,
\n→\\n) - Converting them to spaces
- Removing them entirely
This would prevent log injection while still maintaining readability of error messages.
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | |
| if (printable.length > maxLength) { | |
| printable.take(maxLength) + "... (truncated)" | |
| } else { | |
| printable | |
| // Remove non-printable characters except tab, then escape newlines and carriage returns | |
| val filtered = msg.filter(c => c >= 32 && c < 127 || c == '\t') | |
| val escaped = filtered | |
| .replace("\n", "\\n") | |
| .replace("\r", "\\r") | |
| if (escaped.length > maxLength) { | |
| escaped.take(maxLength) + "... (truncated)" | |
| } else { | |
| escaped |
| * This prevents log pollution from malformed HTTP requests. | ||
| */ | ||
| private def sanitizeErrorMessage(msg: String, maxLength: Int = 200): String = { | ||
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') |
There was a problem hiding this comment.
The filter condition has incorrect operator precedence. The condition c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t' will be evaluated as (c >= 32 && c < 127) || c == '\n' || c == '\r' || c == '\t', which means tab, newline, and carriage return characters will always pass the filter even if they're outside the printable range. While these specific characters are within ASCII range 0-32 and this works correctly by accident, the intent should be made clear with explicit parentheses for maintainability. Consider rewriting as: c >= 32 && c < 127 || (c == '\n' || c == '\r' || c == '\t')
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | |
| val printable = msg.filter(c => (c >= 32 && c < 127) || (c == '\n' || c == '\r' || c == '\t')) |
| * This prevents log pollution from malformed HTTP requests with non-printable characters. | ||
| */ | ||
| private def sanitizeErrorMessage(msg: String, maxLength: Int = 200): String = { | ||
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') |
There was a problem hiding this comment.
The filter condition has incorrect operator precedence. The condition c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t' will be evaluated as (c >= 32 && c < 127) || c == '\n' || c == '\r' || c == '\t', which means tab, newline, and carriage return characters will always pass the filter even if they're outside the printable range. While these specific characters are within ASCII range 0-32 and this works correctly by accident, the intent should be made clear with explicit parentheses for maintainability. Consider rewriting as: c >= 32 && c < 127 || (c == '\n' || c == '\r' || c == '\t')
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | |
| val printable = msg.filter(c => (c >= 32 && c < 127) || (c == '\n' || c == '\r' || c == '\t')) |
| private def sanitizeErrorMessage(msg: String, maxLength: Int = 200): String = { | ||
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | ||
| if (printable.length > maxLength) { | ||
| printable.take(maxLength) + "... (truncated)" | ||
| } else { | ||
| printable | ||
| } | ||
| } |
There was a problem hiding this comment.
The sanitizeErrorMessage function is duplicated between ApiRejectionHandler and ApiErrorHandler with identical implementations. This code duplication violates the DRY principle and makes maintenance harder—if the sanitization logic needs to be updated, it must be changed in two places. Consider extracting this function into a shared utility object or trait that both handlers can use.
| private def sanitizeErrorMessage(msg: String, maxLength: Int = 200): String = { | ||
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | ||
| if (printable.length > maxLength) { | ||
| printable.take(maxLength) + "... (truncated)" | ||
| } else { | ||
| printable | ||
| } | ||
| } |
There was a problem hiding this comment.
The sanitizeErrorMessage function is duplicated between ApiRejectionHandler and ApiErrorHandler with identical implementations. This code duplication violates the DRY principle and makes maintenance harder—if the sanitization logic needs to be updated, it must be changed in two places. Consider extracting this function into a shared utility object or trait that both handlers can use.
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | ||
| if (printable.length > maxLength) { | ||
| printable.take(maxLength) + "... (truncated)" | ||
| } else { | ||
| printable | ||
| } | ||
| } |
There was a problem hiding this comment.
The sanitizeErrorMessage function doesn't handle null input. If e.getMessage returns null (which can happen with some exceptions), calling filter on it will throw a NullPointerException. Consider adding a null check at the beginning of the function, such as: if (msg == null) return "null" or Option(msg).getOrElse("").
| val printable = msg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | |
| if (printable.length > maxLength) { | |
| printable.take(maxLength) + "... (truncated)" | |
| } else { | |
| printable | |
| } | |
| } | |
| val safeMsg = if (msg == null) "null" else msg | |
| val printable = safeMsg.filter(c => c >= 32 && c < 127 || c == '\n' || c == '\r' || c == '\t') | |
| if (printable.length > maxLength) { | |
| printable.take(maxLength) + "... (truncated)" | |
| } else { | |
| printable | |
| } |
Issue #1318: Improve HTTP Error Handling - Implementation Summary
Overview
This PR addresses Issue #1318 by implementing sanitized error logging for malformed HTTP requests, preventing non-printable characters from cluttering logs when invalid requests like "HTTP method too long" are received.
Problem Statement
When the Ergo node receives malformed HTTP requests (e.g., HTTPS connections to HTTP endpoints, or requests with excessively long/invalid methods), Akka HTTP logs error messages containing non-printable characters:
These non-printable characters can:
Solution
1. Sanitization Function (
ApiRejectionHandler.scala&ApiErrorHandler.scala)Added a
sanitizeErrorMessage()function that:2. Enhanced Exception Handling (
ApiErrorHandler.scala)IllegalRequestException(which includes "HTTP method too long")3. Improved Rejection Handling (
ApiRejectionHandler.scala)MalformedRequestContentRejection: Sanitizes malformed JSON/XML contentValidationRejection: Sanitizes validation error messagesBenefits
1. Clean Logs
2. Security
3. Performance
4. Backward Compatibility
Testing Recommendations
Malformed HTTP Requests:
Verify Logs:
Normal Requests:
Files Modified
src/main/scala/scorex/core/api/http/ApiErrorHandler.scala- Enhanced exception handling with sanitizationsrc/main/scala/scorex/core/api/http/ApiRejectionHandler.scala- Added sanitization to rejection handlingRelated Issues
Discussion Points
From the GitHub issue discussion:
This implementation:
✅ Handles the issue at the application level (no akka-http changes needed)
✅ Preserves error information while sanitizing output
✅ Uses appropriate log levels (debug for expected malformed requests)
✅ Can be extended with unit tests for various malformed request types
🎥 Video Demonstration
Screen.Recording.2025-12-14.at.10.09.57.AM.mov
Test Script Included
An automated test script (
test_issue_1318.sh) is provided to verify the fix:Key Test Scenarios
Test Case 1: Binary Data in HTTP Request
þ¸...Test Case 2: 500-Character HTTP Method
Test Case 3: HTTPS to HTTP Port
Video Demonstration Highlights
sanitizeErrorMessage()functionVerification Commands
Future Enhancements
Consider: