Skip to content

Task/#343 split out logger #346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 10, 2025
Merged

Task/#343 split out logger #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b254333
fix logger bug with correlation id
StuartFerguson Jul 10, 2025
df75635
Merge branch 'task/#343_split_out_logger' of ssh://github.com/StuartF…
StuartFerguson Jul 10, 2025
f21c72a
Merge branch 'master' into task/#343_split_out_logger
StuartFerguson Jul 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions Shared.Logger/Logger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

[ExcludeFromCodeCoverage]
public static class Logger {
private static ILogger LoggerObject;

Check warning on line 11 in Shared.Logger/Logger.cs

View workflow job for this annotation

GitHub Actions / Build and Test Pull Requests - Windows 

Non-nullable field 'LoggerObject' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the field as nullable.

Check warning on line 11 in Shared.Logger/Logger.cs

View workflow job for this annotation

GitHub Actions / Build and Test Pull Requests - Windows 

Non-nullable field 'LoggerObject' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the field as nullable.

Check warning on line 11 in Shared.Logger/Logger.cs

View workflow job for this annotation

GitHub Actions / Build and Test Pull Requests - Linux 

Non-nullable field 'LoggerObject' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the field as nullable.

Check warning on line 11 in Shared.Logger/Logger.cs

View workflow job for this annotation

GitHub Actions / Build and Test Pull Requests - Linux 

Non-nullable field 'LoggerObject' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the field as nullable.

public static Boolean IsInitialised { get; set; }

Expand Down Expand Up @@ -36,6 +36,10 @@

TenantContext tenantContext = TenantContext.CurrentTenant;

if (tenantContext == null) {
LoggerObject.LogCritical(exception);
return;
}
using (ScopeContext.PushProperty("correlationId", $"Correlation ID: {tenantContext.CorrelationId.ToString()}")) {
// Write to the normal log
LoggerObject.LogCritical(exception);
Expand All @@ -47,11 +51,18 @@
}
}
}

}

public static void LogDebug(String message) {
ValidateLoggerObject();
TenantContext tenantContext = TenantContext.CurrentTenant;

if (tenantContext == null) {
LoggerObject.LogDebug(message);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 4 months ago

Copilot could not generate an autofix suggestion

Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.

return;
}

using (ScopeContext.PushProperty("correlationId", $"Correlation ID: {tenantContext.CorrelationId.ToString()}")) {
// Write to the normal log
LoggerObject.LogDebug(message);
Expand All @@ -69,6 +80,12 @@
ValidateLoggerObject();

TenantContext tenantContext = TenantContext.CurrentTenant;

if (tenantContext == null) {
LoggerObject.LogError(exception);
return;
}

using (ScopeContext.PushProperty("correlationId", $"Correlation ID: {tenantContext.CorrelationId.ToString()}")) {
// Write to the normal log
LoggerObject.LogError(exception);
Expand All @@ -87,6 +104,10 @@
ValidateLoggerObject();

TenantContext tenantContext = TenantContext.CurrentTenant;
if (tenantContext == null) {
LoggerObject.LogError(message, exception);
return;
}
using (ScopeContext.PushProperty("correlationId", $"Correlation ID: {tenantContext.CorrelationId.ToString()}")) {
// Write to the normal log
LoggerObject.LogError(message, exception);
Expand All @@ -104,6 +125,10 @@
ValidateLoggerObject();

TenantContext tenantContext = TenantContext.CurrentTenant;
if (tenantContext == null) {
Logger.LoggerObject.LogInformation(message);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 4 months ago

Copilot could not generate an autofix suggestion

Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.

return;
}
using (ScopeContext.PushProperty("correlationId", $"Correlation ID: {tenantContext.CorrelationId.ToString()}")) {
// Write to the normal log
Logger.LoggerObject.LogInformation(message);
Expand All @@ -121,6 +146,10 @@
ValidateLoggerObject();

TenantContext tenantContext = TenantContext.CurrentTenant;
if (tenantContext == null) {
Logger.LoggerObject.LogTrace(message);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 4 months ago

To fix the issue, we need to sanitize the message parameter before it is logged. Since the log entries are plain text, we should remove any newline characters from the message to prevent log forging. This can be achieved using String.Replace to replace newline characters (\n and \r) with an empty string. The sanitization should be applied in the Helpers.LogMessage method, as it is the central point where the message is passed to the logging methods.

Suggested changeset 1
Shared/Middleware/Helpers.cs
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Shared/Middleware/Helpers.cs b/Shared/Middleware/Helpers.cs
--- a/Shared/Middleware/Helpers.cs
+++ b/Shared/Middleware/Helpers.cs
@@ -15,6 +15,8 @@
     {
+        // Sanitize the message to remove newline characters
+        String sanitizedMessage = message.ToString().Replace("\r", "").Replace("\n", "");
         String logMessage = Helpers.IsHealthCheckRequest(url) switch
         {
-            true => $"HEALTH_CHECK | {message}",
-            _ => message.ToString()
+            true => $"HEALTH_CHECK | {sanitizedMessage}",
+            _ => sanitizedMessage
         };
EOF
@@ -15,6 +15,8 @@
{
// Sanitize the message to remove newline characters
String sanitizedMessage = message.ToString().Replace("\r", "").Replace("\n", "");
String logMessage = Helpers.IsHealthCheckRequest(url) switch
{
true => $"HEALTH_CHECK | {message}",
_ => message.ToString()
true => $"HEALTH_CHECK | {sanitizedMessage}",
_ => sanitizedMessage
};
Copilot is powered by AI and may make mistakes. Always verify output.
return;
}
using (ScopeContext.PushProperty("correlationId", $"Correlation ID: {tenantContext.CorrelationId.ToString()}")) {
// Write to the normal log
Logger.LoggerObject.LogTrace(message);
Expand All @@ -138,6 +167,10 @@
ValidateLoggerObject();

TenantContext tenantContext = TenantContext.CurrentTenant;
if (tenantContext == null) {
Logger.LoggerObject.LogWarning(message);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 4 months ago

To fix the issue, we need to sanitize the message parameter before it is logged. This involves removing potentially harmful characters, such as newlines, or encoding the input to prevent log forging or injection attacks. Since the logs are plain text, we can use String.Replace to remove newline characters (\n and \r) from the message. This ensures that malicious input cannot create new log entries or disrupt the log format.

The sanitization should be applied consistently across all logging methods (LogInformation, LogTrace, LogWarning, etc.) in the Logger class. Additionally, the Helpers.LogMessage method should sanitize the message parameter before passing it to the logger.

Suggested changeset 2
Shared.Logger/Logger.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Shared.Logger/Logger.cs b/Shared.Logger/Logger.cs
--- a/Shared.Logger/Logger.cs
+++ b/Shared.Logger/Logger.cs
@@ -126,2 +126,4 @@
 
+            message = message.Replace("\n", "").Replace("\r", "");
+
             TenantContext tenantContext = TenantContext.CurrentTenant;
@@ -147,2 +149,4 @@
 
+            message = message.Replace("\n", "").Replace("\r", "");
+
             TenantContext tenantContext = TenantContext.CurrentTenant;
@@ -168,2 +172,4 @@
 
+            message = message.Replace("\n", "").Replace("\r", "");
+
             TenantContext tenantContext = TenantContext.CurrentTenant;
EOF
@@ -126,2 +126,4 @@

message = message.Replace("\n", "").Replace("\r", "");

TenantContext tenantContext = TenantContext.CurrentTenant;
@@ -147,2 +149,4 @@

message = message.Replace("\n", "").Replace("\r", "");

TenantContext tenantContext = TenantContext.CurrentTenant;
@@ -168,2 +172,4 @@

message = message.Replace("\n", "").Replace("\r", "");

TenantContext tenantContext = TenantContext.CurrentTenant;
Shared/Middleware/Helpers.cs
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Shared/Middleware/Helpers.cs b/Shared/Middleware/Helpers.cs
--- a/Shared/Middleware/Helpers.cs
+++ b/Shared/Middleware/Helpers.cs
@@ -15,6 +15,7 @@
     {
+        String sanitizedMessage = message.ToString().Replace("\n", "").Replace("\r", "");
         String logMessage = Helpers.IsHealthCheckRequest(url) switch
         {
-            true => $"HEALTH_CHECK | {message}",
-            _ => message.ToString()
+            true => $"HEALTH_CHECK | {sanitizedMessage}",
+            _ => sanitizedMessage
         };
EOF
@@ -15,6 +15,7 @@
{
String sanitizedMessage = message.ToString().Replace("\n", "").Replace("\r", "");
String logMessage = Helpers.IsHealthCheckRequest(url) switch
{
true => $"HEALTH_CHECK | {message}",
_ => message.ToString()
true => $"HEALTH_CHECK | {sanitizedMessage}",
_ => sanitizedMessage
};
Copilot is powered by AI and may make mistakes. Always verify output.
return;
}
using (ScopeContext.PushProperty("correlationId", $"Correlation ID: {tenantContext.CorrelationId.ToString()}")) {
// Write to the normal log
Logger.LoggerObject.LogWarning(message);
Expand Down
2 changes: 1 addition & 1 deletion Shared.Logger/TennantContext/TennatContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

public static TenantContext CurrentTenant
{
get => TenantContext.Current?.Value ?? new TenantContext();
get => TenantContext.Current?.Value;

Check warning on line 30 in Shared.Logger/TennantContext/TennatContext.cs

View workflow job for this annotation

GitHub Actions / Build and Test Pull Requests - Windows 

Possible null reference return.

Check warning on line 30 in Shared.Logger/TennantContext/TennatContext.cs

View workflow job for this annotation

GitHub Actions / Build and Test Pull Requests - Windows 

Possible null reference return.

Check warning on line 30 in Shared.Logger/TennantContext/TennatContext.cs

View workflow job for this annotation

GitHub Actions / Build and Test Pull Requests - Linux 

Possible null reference return.

Check warning on line 30 in Shared.Logger/TennantContext/TennatContext.cs

View workflow job for this annotation

GitHub Actions / Build and Test Pull Requests - Linux 

Possible null reference return.
set => TenantContext.Current.Value = value;
}

Expand Down
Loading