LNK-3044: Log Forging Query Dispatch #526
Conversation
WalkthroughThe changes in this pull request involve multiple classes within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (12)
DotNet/QueryDispatch/Jobs/QueryDispatchJob.cs (3)
Line range hint
108-121: Security: Audit message needs sanitization.The audit message contains unsanitized facility ID which could lead to log injection vulnerabilities.
Apply this diff to sanitize the facility ID in the audit message:
var auditMessage = new AuditEventMessage { - FacilityId = patientDispatchEntity.FacilityId, + FacilityId = HtmlInputSanitizer.Sanitize(patientDispatchEntity.FacilityId), ServiceName = QueryDispatchConstants.ServiceName, Action = AuditEventType.Create, EventDate = DateTime.UtcNow, Resource = nameof(KafkaTopic.DataAcquisitionRequested), - Notes = $"Produced Data Acquisition Event for facilityId: {patientDispatchEntity.FacilityId}" + Notes = $"Produced Data Acquisition Event for facilityId: {HtmlInputSanitizer.Sanitize(patientDispatchEntity.FacilityId)}" };
Line range hint
47-82: Security: Data acquisition message needs sanitization.The DataAcquisitionRequestedValue contains unsanitized identifiers that could potentially be logged downstream.
Apply this diff to sanitize the identifiers:
DataAcquisitionRequestedValue dataAcquisitionRequestedValue = new DataAcquisitionRequestedValue() { - PatientId = patientDispatchEntity.PatientId, + PatientId = HtmlInputSanitizer.Sanitize(patientDispatchEntity.PatientId), ScheduledReports = new List<ScheduledReport>(), QueryType = QueryTypes.Initial.ToString() }; foreach (var scheduledReportPeriod in patientDispatchEntity.ScheduledReportPeriods) { dataAcquisitionRequestedValue.ScheduledReports.Add(new ScheduledReport { - ReportType = scheduledReportPeriod.ReportType, + ReportType = HtmlInputSanitizer.Sanitize(scheduledReportPeriod.ReportType), StartDate = scheduledReportPeriod.StartDate, EndDate = scheduledReportPeriod.EndDate }); } var acqHeaders = new Headers { { "X-Correlation-Id", Encoding.UTF8.GetBytes(patientDispatchEntity.CorrelationId) } }; _acquisitionProducer.Produce(nameof(KafkaTopic.DataAcquisitionRequested), new Message<string, DataAcquisitionRequestedValue> { - Key = patientDispatchEntity.FacilityId, + Key = HtmlInputSanitizer.Sanitize(patientDispatchEntity.FacilityId), Value = dataAcquisitionRequestedValue, Headers = acqHeaders });
Error logging needs sanitization for user-controlled data
The error logging in the catch block includes the raw exception which may contain user-controlled data from patientDispatchEntity that should be sanitized before logging, similar to how it's sanitized in the info logging above.
- In
QueryDispatchJob.cs, sanitize user-controlled data in error logging:_logger.LogError(ex, "Failed to generate Data Acquisition Requested event for facilityId: {0}", HtmlInputSanitizer.Sanitize(patientDispatchEntity.FacilityId));🔗 Analysis chain
Line range hint
1-121: Verify error logging sanitization.The error logging in the catch block might need sanitization if it includes any user-controlled data.
Let's check if there are any other instances of unsanitized logging in error handlers:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for logging patterns in catch blocks rg -U "catch.*\{.*_logger\." --type csLength of output: 40
Script:
#!/bin/bash # Search for error logging patterns in catch blocks with broader pattern and context rg -U "catch.*?\{[\s\S]*?LogError" -A 5 --type csLength of output: 51
Script:
#!/bin/bash # Let's try a simpler pattern to find catch blocks first rg "catch\s*\([^)]*\)" --type cs -B 2 -A 5Length of output: 345067
Script:
#!/bin/bash # Let's check if the error logging in QueryDispatchJob.cs is sanitized rg -A 5 "LogError.*Failed to generate a Data Acquisition Requested event" DotNet/QueryDispatch/Jobs/QueryDispatchJob.csLength of output: 234
DotNet/QueryDispatch/Domain/Managers/PatientDispatchManager.cs (1)
Line range hint
108-114: Security Issue: Audit message contains unsanitized data.Similar to the create method, the audit message contains unsanitized values for
FacilityIdand notes.Apply this fix:
var auditMessage = new AuditEventMessage { - FacilityId = facilityId, + FacilityId = HtmlInputSanitizer.Sanitize(facilityId), ServiceName = QueryDispatchConstants.ServiceName, Action = AuditEventType.Delete, EventDate = DateTime.UtcNow, Resource = typeof(PatientDispatchEntity).Name, - Notes = $"Deleted Patient Dispatch record for patient id {patientId} in facility {facilityId}" + Notes = $"Deleted Patient Dispatch record for patient id {HtmlInputSanitizer.Sanitize(patientId)} in facility {HtmlInputSanitizer.Sanitize(facilityId)}" };DotNet/QueryDispatch/Domain/Managers/ScheduledReportManager.cs (4)
50-50: Fix typo in log message.The word "faciltiy" is misspelled in the log message.
-_logger.LogInformation($"Created schedule report for faciltiy {HtmlInputSanitizer.Sanitize(scheduledReport.FacilityId)}"); +_logger.LogInformation($"Created schedule report for facility {HtmlInputSanitizer.Sanitize(scheduledReport.FacilityId)}");
Line range hint
58-77: Sanitize FacilityId in audit message.The FacilityId in the audit message is not sanitized, which could lead to log forging in the audit trail.
var auditMessage = new AuditEventMessage { - FacilityId = scheduledReport.FacilityId, + FacilityId = HtmlInputSanitizer.Sanitize(scheduledReport.FacilityId), ServiceName = QueryDispatchConstants.ServiceName, Action = AuditEventType.Create, EventDate = DateTime.UtcNow, Resource = typeof(ScheduledReportEntity).Name, - Notes = $"Created schedule report {scheduledReport.Id} for facility {scheduledReport.FacilityId} " + Notes = $"Created schedule report {scheduledReport.Id} for facility {HtmlInputSanitizer.Sanitize(scheduledReport.FacilityId)} " };
81-82: Consider using a sanitized exception message wrapper.Throwing exceptions with unsanitized data in the message could expose sensitive information or enable injection attacks if the exception messages are logged or displayed elsewhere in the application.
Consider creating a custom exception type that internally handles sanitization:
public class SanitizedApplicationException : ApplicationException { public SanitizedApplicationException(string message, params object[] args) : base(string.Format(message, args.Select(arg => HtmlInputSanitizer.Sanitize(arg?.ToString())).ToArray())) { } }Then use it like:
-throw new ApplicationException($"Failed to create scheduled report for facility {HtmlInputSanitizer.Sanitize(scheduledReport.FacilityId)}."); +throw new SanitizedApplicationException("Failed to create scheduled report for facility {0}", scheduledReport.FacilityId);
Line range hint
136-157: Sanitize FacilityId in audit message.The FacilityId and Notes in the audit message are not sanitized, which could lead to log forging in the audit trail.
var auditMessage = new AuditEventMessage { - FacilityId = existingReport.FacilityId, + FacilityId = HtmlInputSanitizer.Sanitize(existingReport.FacilityId), ServiceName = QueryDispatchConstants.ServiceName, Action = AuditEventType.Update, EventDate = DateTime.UtcNow, PropertyChanges = propertyChanges, Resource = typeof(ScheduledReportEntity).Name, - Notes = $"Updated schedule report {existingReport.Id} for facility {existingReport.FacilityId}" + Notes = $"Updated schedule report {existingReport.Id} for facility {HtmlInputSanitizer.Sanitize(existingReport.FacilityId)}" };DotNet/QueryDispatch/Domain/Managers/QueryDispatchConfigurationManager.cs (2)
Line range hint
144-148: Consider improving not-found handling.The method silently returns when the configuration is not found. Consider logging this scenario or throwing a more specific exception to help with troubleshooting.
if(config == null) { + _logger.LogWarning($"No query dispatch configuration found for facility {HtmlInputSanitizer.Sanitize(facilityId)}"); return; }
Audit message construction needs sanitization for facilityId
The logging statements properly sanitize facilityId using
HtmlInputSanitizer.Sanitize(), but the audit messages directly use unsanitized facilityId values:
- Line ~89:
FacilityId = config.FacilityIdin Update audit message- Line ~120:
FacilityId = config.FacilityIdin Create audit message- Line ~160:
FacilityId = facilityIdin Delete audit message🔗 Analysis chain
Line range hint
1-190: Verify complete sanitization coverage.Let's verify that all instances of facilityId in logging statements are properly sanitized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any logging calls with unsanitized facilityId rg -i 'log\w*\s*\([^)]*facility.*?(?<!Sanitize\()(?:config\.)?facilityId' # Search for any string interpolation with facilityId that might be used in logging rg '\$".*?(?<!Sanitize\()(?:config\.)?facilityId'Length of output: 679
Script:
#!/bin/bash # Search for all logging statements with facilityId rg -A 2 '_logger\.Log\w+\(' DotNet/QueryDispatch/Domain/Managers/QueryDispatchConfigurationManager.cs # Search for all audit message constructions rg -A 5 'var auditMessage = new AuditEventMessage' DotNet/QueryDispatch/Domain/Managers/QueryDispatchConfigurationManager.csLength of output: 2604
DotNet/QueryDispatch/Presentation/Controllers/QueryDispatchController.cs (2)
73-73: Consider using structured logging format consistently.While the sanitization is correctly implemented, consider using the structured logging format consistently:
-_logger.LogError(new EventId(QueryDispatchConstants.LoggingIds.GetItem, "Get QueryDispatch configuration"), ex, "An exception occurred while attempting to retrieve a QueryDispatch configuration for facility {facilityId}", HtmlInputSanitizer.Sanitize(facilityId)); +_logger.LogError(new EventId(QueryDispatchConstants.LoggingIds.GetItem, "Get QueryDispatch configuration"), ex, "An exception occurred while attempting to retrieve a QueryDispatch configuration. FacilityId: {facilityId}", HtmlInputSanitizer.Sanitize(facilityId));
Line range hint
1-258: Security Review: Log sanitization implementation is solid but needs consistency.The implementation successfully addresses log forging by sanitizing all user inputs before logging. However, there are some areas for improvement:
- Standardize on structured logging instead of string concatenation
- Review the logging of serialized models for potential sensitive data exposure
- Consider extracting common validation and logging patterns
The core security enhancement is sound, but these improvements would make the implementation more robust and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- DotNet/QueryDispatch/Domain/Managers/PatientDispatchManager.cs (6 hunks)
- DotNet/QueryDispatch/Domain/Managers/QueryDispatchConfigurationManager.cs (7 hunks)
- DotNet/QueryDispatch/Domain/Managers/ScheduledReportManager.cs (5 hunks)
- DotNet/QueryDispatch/Jobs/QueryDispatchJob.cs (2 hunks)
- DotNet/QueryDispatch/Listeners/PatientEventListener.cs (4 hunks)
- DotNet/QueryDispatch/Presentation/Controllers/QueryDispatchController.cs (7 hunks)
🔇 Additional comments (16)
DotNet/QueryDispatch/Jobs/QueryDispatchJob.cs (2)
12-12: LGTM! Security namespace addition.The addition of the security namespace is appropriate for implementing log sanitization.
83-83: LGTM! Proper sanitization of facility ID in log message.The FacilityId is correctly sanitized using HtmlInputSanitizer before logging.
DotNet/QueryDispatch/Domain/Managers/PatientDispatchManager.cs (2)
8-8: LGTM: Security namespace import added correctly.The addition of the security namespace import is appropriate for the log sanitization changes.
Line range hint
89-91: Security: Verify input sanitization for database queries.The database query uses unsanitized
facilityIdandpatientIddirectly in the LINQ expression.Let's check if the repository layer handles input sanitization:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Input sanitization is properly implemented for database operations
The code shows proper input sanitization and validation:
- The
PatientDispatchManagerusesHtmlInputSanitizer.Sanitize()for all user inputs before logging.- The database operations are performed using Entity Framework Core's parameterized queries:
FirstOrDefaultAsync(x => x.FacilityId == facilityId && x.PatientId == patientId)uses parameters internallyRemoveAsync(entity)operates on the entity object, not raw SQL- The
PatientDispatchEntityis properly defined with EF Core's table and property mappings, ensuring safe database operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the repository implementation to verify sanitization ast-grep --pattern 'class $_ : IEntityRepository<PatientDispatchEntity>'Length of output: 74
Script:
#!/bin/bash # Let's try a broader search for repository implementations and entity definitions echo "=== Searching for repository interface and implementations ===" rg -l "IEntityRepository" --type cs echo -e "\n=== Searching for PatientDispatchEntity definition ===" rg -l "PatientDispatchEntity" --type cs echo -e "\n=== Looking for RemoveAsync implementation ===" ast-grep --pattern 'RemoveAsync($$$) { $$$ }' echo -e "\n=== Checking for any database context or query sanitization ===" rg -B 2 -A 2 "RemoveAsync|DeleteAsync" --type csLength of output: 35174
Script:
#!/bin/bash # Let's check for input sanitization in the PatientDispatchManager and related components echo "=== Checking PatientDispatchManager implementation ===" rg -B 5 -A 5 "facilityId.*patientId" --type cs echo -e "\n=== Checking for any input validation or sanitization utilities ===" rg -l "HtmlInputSanitizer|InputValidator|Sanitize|Validate" --type cs echo -e "\n=== Checking QueryDispatchDbContext for any entity configurations ===" rg -B 5 -A 5 "PatientDispatchEntity" DotNet/QueryDispatch/Domain/Context/QueryDispatchDbContext.cs echo -e "\n=== Looking for PatientDispatchEntity definition ===" rg -B 5 -A 5 "class PatientDispatchEntity" --type csLength of output: 35041
DotNet/QueryDispatch/Domain/Managers/ScheduledReportManager.cs (3)
8-8: LGTM! Security import added correctly.The addition of the security services import aligns with the PR's objective of preventing log forging.
162-163: Consider using a sanitized exception message wrapper.Similar to the createScheduledReport method, consider using the SanitizedApplicationException to handle message sanitization consistently.
Line range hint
91-107: Verify sanitization of property changes in audit messages.The property changes captured for audit messages might contain sensitive data that needs sanitization. Please verify if any of the compared properties contain sensitive information that should be sanitized before being added to the audit trail.
✅ Verification successful
Let me gather more information about the ReportPeriodEntity to understand what properties it contains.
Based on the findings, I can now generate the final response:
No sensitive data found in ReportPeriodEntity properties
The ReportPeriodEntity only contains basic scheduling metadata:
- ReportType (string)
- StartDate (DateTime)
- EndDate (DateTime)
- CreateDate (DateTime)
These properties do not contain sensitive information that would require sanitization in audit logs. The current implementation correctly excludes timestamp fields (createdate, modifydate) from the audit trail, which is appropriate for change tracking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of CompareLogic to understand what properties are being compared rg -A 5 "CompareLogic" --type csLength of output: 6453
Script:
#!/bin/bash # Search for ReportPeriodEntity definition to understand its properties ast-grep --pattern 'class ReportPeriodEntity { $$$ }' # Also search for any sensitive data annotations or attributes rg -B 2 -A 5 "ReportPeriodEntity" --type csLength of output: 6746
DotNet/QueryDispatch/Domain/Managers/QueryDispatchConfigurationManager.cs (1)
10-10: LGTM! Security enhancement for log sanitization.The addition of the HtmlInputSanitizer import aligns with the PR's objective of addressing log forging vulnerabilities.
DotNet/QueryDispatch/Presentation/Controllers/QueryDispatchController.cs (4)
7-7: LGTM: Security service import added for log sanitization.The addition of the security services import is appropriate for implementing log sanitization.
110-111: LGTM: Consistent sanitization of duration in logs and response.The duration value is properly sanitized in both the log message and the response message.
203-203: Review model parameter in log message.The log message includes a model parameter that might contain sensitive data. Consider logging only necessary fields:
#!/bin/bash # Check for other logging of the model parameter rg "model}" -A 5 -B 5
102-102: Review JSON serialization security implications.While the serialized JSON is being sanitized, consider whether all sensitive data in the model should be logged. Consider implementing a sanitized version of the model for logging purposes.
✅ Verification successful
No sensitive data found in QueryDispatchConfiguration model
After reviewing the model structure, its base classes, and related entities, the QueryDispatchConfiguration only contains basic operational data (FacilityId and DispatchSchedules with event type and duration). The current logging implementation with sanitization is appropriate as there are no sensitive fields that need to be excluded from logs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for sensitive fields in QueryDispatchConfiguration rg -A 5 "class QueryDispatchConfiguration"Length of output: 6143
Script:
#!/bin/bash # Search for QueryDispatchConfiguration class definition ast-grep --pattern 'class QueryDispatchConfiguration { $$$ }' # Also check for any DTO or model classes that might inherit/implement QueryDispatchConfiguration rg -l "QueryDispatchConfiguration"Length of output: 1481
Script:
#!/bin/bash # Check the model definition for sensitive fields cat DotNet/QueryDispatch/Application/Models/QueryDispatchConfiguration.cs # Check if there are any sensitive data annotations or attributes rg -A 2 "Sensitive|Password|Secret|Key|Token|Credential" DotNet/QueryDispatch/Application/Models/QueryDispatchConfiguration.csLength of output: 501
Script:
#!/bin/bash # Check the DispatchSchedule model for potential sensitive fields cat DotNet/QueryDispatch/Domain/Entities/DispatchSchedule.cs # Also check if there are any sensitive fields in the entity cat DotNet/QueryDispatch/Domain/Entities/QueryDispatchConfigurationEntity.csLength of output: 1103
Script:
#!/bin/bash # Check the base entity class for any additional fields cat DotNet/QueryDispatch/Domain/Entities/BaseQueryEntity.cs # Check if there are any sensitive configurations or settings related to this model rg -A 5 "class QueryDispatchConstants"Length of output: 1089
DotNet/QueryDispatch/Listeners/PatientEventListener.cs (4)
12-12: LGTM: Security import added correctly.The security namespace import is appropriately placed and necessary for the log sanitization improvements.
129-143: LGTM: Exception messages properly sanitized.All instances of facility ID in exception messages are correctly sanitized, preventing potential log forging through exception messages.
111-111:⚠️ Potential issueConsider sanitizing the correlation ID.
While facility ID, patient ID, and event type are properly sanitized, the correlation ID extracted from headers is not sanitized when used in logs or error messages. Consider applying the same sanitization to the correlation ID to maintain consistent security practices.
Apply this pattern when using the correlation ID:
- correlationId = System.Text.Encoding.UTF8.GetString(headerValue); + correlationId = HtmlInputSanitizer.Sanitize(System.Text.Encoding.UTF8.GetString(headerValue));Likely invalid or redundant comment.
Line range hint
1-208: Verify consistent sanitization across the codebase.Let's verify that similar sanitization patterns are consistently applied across other files in the codebase.
DotNet/QueryDispatch/Domain/Managers/QueryDispatchConfigurationManager.cs
Show resolved
Hide resolved
DotNet/QueryDispatch/Domain/Managers/QueryDispatchConfigurationManager.cs
Show resolved
Hide resolved
DotNet/QueryDispatch/Presentation/Controllers/QueryDispatchController.cs
Show resolved
Hide resolved
DotNet/QueryDispatch/Presentation/Controllers/QueryDispatchController.cs
Show resolved
Hide resolved
DotNet/QueryDispatch/Presentation/Controllers/QueryDispatchController.cs
Show resolved
Hide resolved
DotNet/QueryDispatch/Presentation/Controllers/QueryDispatchController.cs
Show resolved
Hide resolved
edward-miller-lcg
changed the title
🛠 Description of Changes
Please provide a high-level overview of the changes included in this PR.
🧪 Testing Performed
Please describe the testing that was performed on the changes included in this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation