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 ResetChecker(string checkerName, CancellationTo } /// - /// Strips control characters from a name before it is written to a log. + /// Removes the characters that would let a name forge log output. /// /// The name as the route supplied it. - /// The name, with any control character replaced. + /// The name, with those characters replaced. /// + /// /// 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. + /// + /// + /// Chained 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 cs/log-forging + /// alerts standing against code that was already fixed -- and an alert nobody can close is an + /// alert everybody learns to scroll past. + /// /// - 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)]); - } + /// Stands in for a character removed. + /// Replaced rather than dropped, so the log still shows something was taken out. + 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); } + /// + /// A terminal tailing a log acts on escape sequences, and a route can carry one. + /// + [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() {