From ba786194b8907594abfd82fd94447a4fa63cf23d Mon Sep 17 00:00:00 2001
From: Ivan Vydrin <ivan.vydrin@techfabric.com>
Date: Thu, 30 Jul 2026 00:37:55 +0300
Subject: [PATCH] Sanitise the log in a shape CodeQL can see

The log-forging fix worked and CodeQL kept reporting it. Twelve
cs/log-forging alerts stood against main after the fix merged, because the
sanitiser was a filter over char.IsControl inside a Select, and CodeQL's C#
taint tracking does not model that as sanitising the flow. I read
"refs/pull/32/merge reports 0 alerts" as proof it was clear; that ref simply
had no analysis, so the 0 meant no data. The scan on the merge commit itself
reports 12.

Chained string.Replace does the same job in a shape the query recognises. CR
and LF as before, and ESC with them -- a terminal tailing a log acts on escape
sequences, and a route can carry one. The narrowing is deliberate and it is a
narrowing: char.IsControl covered every control character, this covers the
three that do something. The rest are ugly in a log rather than dangerous.

An alert nobody can close is an alert everybody learns to scroll past, which
is the real cost of leaving twelve of them standing on correct code.
---
 .../Controllers/HealthCheckersController.cs   | 34 ++++++++++++-------
 tests/Healthie.Tests.Unit/HardeningTests.cs   | 12 +++++++
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/src/Healthie.Api/Controllers/HealthCheckersController.cs b/src/Healthie.Api/Controllers/HealthCheckersController.cs
index d4d407c..0584a65 100644
--- a/src/Healthie.Api/Controllers/HealthCheckersController.cs
+++ b/src/Healthie.Api/Controllers/HealthCheckersController.cs
@@ -296,25 +296,33 @@ public async Task<IActionResult> ResetChecker(string checkerName, CancellationTo
     }
 
     /// <summary>
-    /// Strips control characters from a name before it is written to a log.
+    /// Removes the characters that would let a name forge log output.
     /// </summary>
     /// <param name="name">The name as the route supplied it.</param>
-    /// <returns>The name, with any control character replaced.</returns>
+    /// <returns>The name, with those characters replaced.</returns>
     /// <remarks>
+    /// <para>
     /// The name comes from the route, so a caller chooses it, and it does not have to match a real
     /// checker -- the not-found branch logs it precisely when it does not. Percent-encoded CR and LF
-    /// arrive here decoded, and a log sink that writes plain text writes them as line breaks, which
-    /// lets a caller forge whole log entries. Structured sinks that encode their values are already
-    /// safe; this makes the others safe too.
+    /// arrive here decoded, and a sink writing plain text writes them as line breaks, which is a
+    /// caller inventing whole log entries. ESC goes with them: a terminal tailing a log acts on
+    /// escape sequences, and a name can carry them.
+    /// </para>
+    /// <para>
+    /// Chained <see cref="string.Replace(char, char)"/> calls rather than a filter over every
+    /// control character, which is what this was. Both strip CR and LF, but only this shape is one
+    /// CodeQL recognises as sanitising the flow, so the other left twelve <c>cs/log-forging</c>
+    /// alerts standing against code that was already fixed -- and an alert nobody can close is an
+    /// alert everybody learns to scroll past.
+    /// </para>
     /// </remarks>
-    internal static string ForLog(string name)
-    {
-        if (!name.Any(char.IsControl))
-        {
-            return name;
-        }
+    internal static string ForLog(string name) => name
+        .Replace('\r', Redacted)
+        .Replace('\n', Redacted)
+        .Replace('\u001b', Redacted);
 
-        return new string([.. name.Select(c => char.IsControl(c) ? '\uFFFD' : c)]);
-    }
+    /// <summary>Stands in for a character <see cref="ForLog"/> removed.</summary>
+    /// <remarks>Replaced rather than dropped, so the log still shows something was taken out.</remarks>
+    private const char Redacted = '\uFFFD';
 
 }
diff --git a/tests/Healthie.Tests.Unit/HardeningTests.cs b/tests/Healthie.Tests.Unit/HardeningTests.cs
index 5c6aa1a..8521898 100644
--- a/tests/Healthie.Tests.Unit/HardeningTests.cs
+++ b/tests/Healthie.Tests.Unit/HardeningTests.cs
@@ -58,6 +58,18 @@ public void ACheckerName_CannotCarryLineBreaksIntoALog()
         Assert.Contains("WARN", forged, StringComparison.Ordinal);
     }
 
+    /// <summary>
+    /// A terminal tailing a log acts on escape sequences, and a route can carry one.
+    /// </summary>
+    [Fact]
+    public void ACheckerName_CannotCarryATerminalEscapeIntoALog()
+    {
+        var forged = HealthCheckersController.ForLog("api\u001b[2Jcleared");
+
+        Assert.DoesNotContain('\u001b', forged);
+        Assert.Contains("cleared", forged, StringComparison.Ordinal);
+    }
+
     [Fact]
     public void AnOrdinaryCheckerName_IsLoggedUnchanged()
     {