375 safe log consumption - #71
Closed
mm-kgi wants to merge 4 commits into
Closed
Conversation
- Added comprehensive unit tests for LogSanitizer - Refactored existing logging statements across the codebase
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Test & Coverage ReportTest Results Summary
Code CoverageUnit Tests Coverage
Minimum allowed line rate is Module Tests Coverage
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces centralized log input sanitization to mitigate log poisoning/injection risks by adding a reusable sanitizer, applying it at key API log call sites, and enabling automatic sanitization for Serilog structured properties via an enricher.
Changes:
- Added
LogSanitizerutility and unit tests to escape control characters and truncate overly long log values. - Added
SanitizingEnricherto Serilog configuration to sanitize structured string properties automatically, plus unit tests. - Updated several log statements to use safer structured patterns and/or sanitize identifiers before logging.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| source/AAS.TwinEngine.DataEngine/ServiceConfiguration/LoggingConfigurationExtension.cs | Adds Serilog SanitizingEnricher to the logging pipeline. |
| source/AAS.TwinEngine.DataEngine/ServiceConfiguration/InfrastructureDependencyInjectionExtensions.cs | Whitespace-only change. |
| source/AAS.TwinEngine.DataEngine/Infrastructure/Providers/TemplateProvider/Services/SubmodelTemplateMappingProvider.cs | Adds ApplicationLogic.Extensions using (no functional change). |
| source/AAS.TwinEngine.DataEngine/Infrastructure/Providers/TemplateProvider/Services/ShellTemplateMappingProvider.cs | Adjusts log property name casing; adds ApplicationLogic.Extensions using. |
| source/AAS.TwinEngine.DataEngine/Infrastructure/Providers/PluginDataProvider/Services/PluginManifestProvider.cs | Adds ApplicationLogic.Extensions using (no functional change). |
| source/AAS.TwinEngine.DataEngine/Infrastructure/Providers/PluginDataProvider/Services/PluginManifestConflictHandler.cs | Adds ApplicationLogic.Extensions using (no functional change). |
| source/AAS.TwinEngine.DataEngine/Infrastructure/Providers/PluginDataProvider/Services/MultiPluginDataHandler.cs | Adds ApplicationLogic.Extensions using (no functional change). |
| source/AAS.TwinEngine.DataEngine/Infrastructure/Providers/PluginDataProvider/Helper/JsonSchemaValidator.cs | Makes error logging safer by avoiding dynamic message templates; minor refactors. |
| source/AAS.TwinEngine.DataEngine/Infrastructure/Logging/SanitizingEnricher.cs | New: Serilog enricher that sanitizes string property values recursively. |
| source/AAS.TwinEngine.DataEngine/ApplicationLogic/Services/SubmodelRepository/SerializationService.cs | Discards return value of MakeSpec; adds ApplicationLogic.Extensions using. |
| source/AAS.TwinEngine.DataEngine/ApplicationLogic/Services/SubmodelRepository/ConceptDescriptionService.cs | Adds ApplicationLogic.Extensions using (no functional change). |
| source/AAS.TwinEngine.DataEngine/ApplicationLogic/Services/AasRegistry/ShellDescriptorService.cs | Adds ApplicationLogic.Extensions using (no functional change). |
| source/AAS.TwinEngine.DataEngine/ApplicationLogic/Extensions/LogSanitizer.cs | New: sanitizer implementation (escape control chars + truncate). |
| source/AAS.TwinEngine.DataEngine/Api/SubmodelRepository/Handler/SubmodelRepositoryHandler.cs | Sanitizes decoded IDs before logging. |
| source/AAS.TwinEngine.DataEngine/Api/SubmodelRepository/Handler/SerializationHandler.cs | Sanitizes IDs before logging (joins sanitized lists for the log message). |
| source/AAS.TwinEngine.DataEngine/Api/SubmodelRegistry/Handler/SubmodelDescriptorHandler.cs | Sanitizes decoded ID before warning log. |
| source/AAS.TwinEngine.DataEngine/Api/AasRepository/Handler/AasRepositoryHandler.cs | Sanitizes decoded ID before logging. |
| source/AAS.TwinEngine.DataEngine.UnitTests/Infrastructure/Logging/SanitizingEnricherTests.cs | New: unit tests for enricher behavior across scalar/sequence/structure/dictionary. |
| source/AAS.TwinEngine.DataEngine.UnitTests/ApplicationLogic/Extensions/LogSanitizerTests.cs | New: unit tests covering escaping and truncation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,6 @@ | |||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Exceptions.Application; | |||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Exceptions.Infrastructure; | |||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Extensions; | |||
Comment on lines
+38
to
+52
| return value switch | ||
| { | ||
| ScalarValue { Value: string s } => new ScalarValue(LogSanitizer.Sanitize(s)), | ||
| SequenceValue seq => new SequenceValue(seq.Elements.Select(SanitizeValue)), | ||
| StructureValue str => new StructureValue( | ||
| str.Properties.Select(p => new LogEventProperty(p.Name, SanitizeValue(p.Value))), | ||
| str.TypeTag), | ||
| DictionaryValue dict => new DictionaryValue( | ||
| dict.Elements.Select(kvp => new KeyValuePair<ScalarValue, LogEventPropertyValue>( | ||
| SanitizeScalar(kvp.Key), SanitizeValue(kvp.Value)))), | ||
| _ => value | ||
| }; | ||
| } | ||
|
|
||
| private static ScalarValue SanitizeScalar(ScalarValue scalar) => scalar.Value is string s ? new ScalarValue(LogSanitizer.Sanitize(s)) : scalar; |
Comment on lines
+19
to
+27
| public static string Sanitize(string? input, int maxLength = DefaultMaxLength) | ||
| { | ||
| if (string.IsNullOrEmpty(input)) | ||
| { | ||
| return string.Empty; | ||
| } | ||
|
|
||
| var capacity = Math.Min(input.Length * 2, maxLength * 2); | ||
| var sb = new System.Text.StringBuilder(capacity); |
Comment on lines
+26
to
+30
| var capacity = Math.Min(input.Length * 2, maxLength * 2); | ||
| var sb = new System.Text.StringBuilder(capacity); | ||
|
|
||
| foreach (var c in input) | ||
| { |
| using System.Text.Json; | ||
|
|
||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Exceptions.Infrastructure; | ||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Extensions; |
| using System.Text.Json.Nodes; | ||
|
|
||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Exceptions.Application; | ||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Extensions; |
Comment on lines
+4
to
5
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Extensions; | ||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Services.AasEnvironment.Providers; |
| using System.Text.RegularExpressions; | ||
|
|
||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Exceptions.Infrastructure; | ||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Extensions; |
| using System.Xml; | ||
|
|
||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Exceptions.Application; | ||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Extensions; |
Comment on lines
+1
to
+2
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Extensions; | ||
| using AAS.TwinEngine.DataEngine.ApplicationLogic.Services.AasEnvironment.Providers; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.