From ff619260babddc7d07b6753e6be5d745127c5619 Mon Sep 17 00:00:00 2001 From: Ivan Vydrin Date: Wed, 29 Jul 2026 21:05:44 +0300 Subject: [PATCH 1/4] Close what a leak-and-security sweep found Six defects, none of them large, all of them reachable. The dashboard's own page is the one place in the library that builds HTML as a string instead of letting Razor build it, and its title went in unencoded. The title is a host's setting rather than a visitor's, so this only bites when a host builds it from something it did not write itself -- a per-tenant label, a value out of a database -- but that is an ordinary thing to do and nothing stopped such a value closing the title element and opening a script one. The page is now built by a method that can be asserted on without a host, which is what makes the test a test rather than a restatement of the encoder. A dashboard component left its handler behind when it was disposed. The service is scoped to a circuit, so it was assumed that letting the circuit end released everything, and that holds exactly while the dashboard is mounted once per circuit. A host that routes to it inside its own layout builds a new component every time the user navigates back to it, on the same circuit, and each of those left a handler that went on being called after its component was torn down. UnsubscribeFromStateChangesAsync is the missing half. Two callers scheduling one checker at once could leave a timer that nothing could stop. Installing a schedule was "cancel the old one, then start the new one" -- two steps, so both callers could install one, and only the last stored was reachable. The other kept triggering, could not be unscheduled because nothing pointed at it any more, and held a linked CancellationTokenSource that was never disposed. A lock is the whole fix; scheduling happens at startup and when somebody changes an interval, so it is nowhere near a hot path. MaxHistoryPageSize was documented in the README and read by nobody -- get_check_history clamped to a hard-coded 200 instead, so a host that lowered it to keep responses small got no such thing. It is taken as an optional constructor parameter so that reading it does not change how the type is built. The relational table-name guard admitted a name with a trailing newline, because in .NET `$` matches immediately before one as well as at the end of the string. Nothing could be smuggled through it -- a lone trailing newline is whitespace to every engine here, and a second one fails -- but a guard whose own error message promises "letters, digits and underscores" should not accept one, so it ends at `\z` now. A checker name from a REST route reached the log with its control characters intact. The not-found branch logs that name precisely when it matches nothing, percent-encoded CR and LF arrive decoded, and a sink writing plain text writes them as line breaks -- which is a caller forging log entries. Only the logging call sites are sanitised; the lookup and the response body still see the name as it arrived. Two things the sweep raised are deliberately not changed here. Healthie.Api requires no authorization unless the host asks for it, and the dashboard's AllowMutations defaults to true: both are documented decisions with real reasons, and changing either is a behaviour break and the maintainer's call, not a cleanup. A checker's exception message is likewise still reported verbatim -- that message is the product. --- CHANGELOG.md | 25 +++++ .../Controllers/HealthCheckersController.cs | 44 +++++--- src/Healthie.Api/Healthie.Api.csproj | 4 + .../Components/HealthieDashboard.razor.cs | Bin 27758 -> 28046 bytes .../Services/HealthieDashboardService.cs | 18 ++++ .../Services/IHealthieDashboardService.cs | 26 ++++- src/Healthie.Dashboard/StartupExtensions.cs | 27 ++++- .../TimerPulseScheduler.cs | 74 ++++++++++--- src/Healthie.Mcp/HealthieTools.cs | 17 ++- .../RelationalDialect.cs | 9 +- .../DashboardServiceTests.cs | 44 ++++++++ tests/Healthie.Tests.Unit/HardeningTests.cs | 98 ++++++++++++++++++ .../Healthie.Tests.Unit.csproj | 1 + tests/Healthie.Tests.Unit/McpToolTests.cs | 25 +++++ .../TimerPulseSchedulerScheduleTests.cs | 49 +++++++++ 15 files changed, 426 insertions(+), 35 deletions(-) create mode 100644 tests/Healthie.Tests.Unit/HardeningTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 65d0987..a614520 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,31 @@ Eleven new packages, and the schedule model that several of them needed. Everyth additive: no public API was removed or changed, and an application that upgrades without touching its code behaves exactly as it did. +### Fixed + +- **The dashboard's own page did not encode its title.** `MapHealthieUI` builds that one page as a + string rather than through Razor, which encodes every interpolation for you, so a + `HealthieUIOptions.DashboardTitle` built from anything the host did not write itself could close + the title element and open a script one. +- **A dashboard component left its handler behind when it was disposed.** The service is scoped to a + circuit, so it was assumed the circuit ending released everything; that holds only while the + dashboard is mounted once per circuit, and a host that routes to it inside its own layout builds a + new one every time the user navigates back. `IHealthieDashboardService.UnsubscribeFromStateChangesAsync` + is the missing half, and the component calls it. +- **Two callers scheduling one checker at once could leave a timer nobody could stop.** Installing a + schedule was "cancel the old one, then start the new one", and only the last one stored was + reachable; the other kept triggering, could not be unscheduled, and held a linked + `CancellationTokenSource` that was never disposed. +- **`HealthieMcpOptions.MaxHistoryPageSize` was documented and never read.** `get_check_history` + clamped to a hard-coded 200 instead, so a host that lowered the option got no such thing. +- **The relational table-name guard admitted a name with a trailing newline.** In .NET `$` matches + immediately before one, so a table name ending in one passed a check whose own error message says + it does not. Nothing could be smuggled through it -- a lone trailing newline is whitespace to + every engine here -- but the guard now ends at `\z` and means what it says. +- **A checker name from a REST route reached the log with its control characters intact.** The + not-found branch logs a name precisely when it matches nothing, percent-encoded CR and LF arrive + decoded, and a log sink writing plain text writes them as line breaks. + ### Added - **Optimistic concurrency on `IStateProvider`.** A state write can now be made conditional on the diff --git a/src/Healthie.Api/Controllers/HealthCheckersController.cs b/src/Healthie.Api/Controllers/HealthCheckersController.cs index ac064df..1f0603a 100644 --- a/src/Healthie.Api/Controllers/HealthCheckersController.cs +++ b/src/Healthie.Api/Controllers/HealthCheckersController.cs @@ -74,6 +74,26 @@ public async Task>> GetChecke /// The new interval to apply. /// A token to monitor for cancellation requests. /// 204 No Content on success, 404 if the checker is not found, or 400 if the name is empty or the interval is not a defined value. + /// + /// Strips control characters from a name before it is written to a log. + /// + /// + /// 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. + /// + internal static string ForLog(string name) + { + if (!name.Any(char.IsControl)) + { + return name; + } + + return new string([.. name.Select(c => char.IsControl(c) ? '�' : c)]); + } + [HttpPut("{checkerName}/interval")] [ProducesResponseType(StatusCodes.Status204NoContent)] [ProducesResponseType(StatusCodes.Status404NotFound)] @@ -99,7 +119,7 @@ public async Task SetCheckerInterval(string checkerName, [FromQue var checkers = await pulsesScheduler.GetPulseCheckersAsync(cancellationToken).ConfigureAwait(false); if (!checkers.ContainsKey(checkerName)) { - logger?.LogWarning("Checker '{CheckerName}' not found for setting interval.", checkerName); + logger?.LogWarning("Checker '{CheckerName}' not found for setting interval.", ForLog(checkerName)); return NotFound($"Checker '{checkerName}' not found."); } @@ -108,7 +128,7 @@ public async Task SetCheckerInterval(string checkerName, [FromQue } catch (Exception ex) { - logger?.LogError(ex, "Error setting interval for checker '{CheckerName}'.", checkerName); + logger?.LogError(ex, "Error setting interval for checker '{CheckerName}'.", ForLog(checkerName)); return StatusCode(StatusCodes.Status500InternalServerError, "An unexpected error occurred."); } } @@ -136,7 +156,7 @@ public async Task SetCheckerThreshold(string checkerName, [FromQu var checkers = await pulsesScheduler.GetPulseCheckersAsync(cancellationToken).ConfigureAwait(false); if (!checkers.ContainsKey(checkerName)) { - logger?.LogWarning("Checker '{CheckerName}' not found for setting threshold.", checkerName); + logger?.LogWarning("Checker '{CheckerName}' not found for setting threshold.", ForLog(checkerName)); return NotFound($"Checker '{checkerName}' not found."); } @@ -145,7 +165,7 @@ public async Task SetCheckerThreshold(string checkerName, [FromQu } catch (Exception ex) { - logger?.LogError(ex, "Error setting threshold for checker '{CheckerName}'.", checkerName); + logger?.LogError(ex, "Error setting threshold for checker '{CheckerName}'.", ForLog(checkerName)); return StatusCode(StatusCodes.Status500InternalServerError, "An unexpected error occurred."); } } @@ -172,7 +192,7 @@ public async Task StartChecker(string checkerName, CancellationTo var checkers = await pulsesScheduler.GetPulseCheckersAsync(cancellationToken).ConfigureAwait(false); if (!checkers.ContainsKey(checkerName)) { - logger?.LogWarning("Checker '{CheckerName}' not found for starting.", checkerName); + logger?.LogWarning("Checker '{CheckerName}' not found for starting.", ForLog(checkerName)); return NotFound($"Checker '{checkerName}' not found."); } @@ -181,7 +201,7 @@ public async Task StartChecker(string checkerName, CancellationTo } catch (Exception ex) { - logger?.LogError(ex, "Error starting checker '{CheckerName}'.", checkerName); + logger?.LogError(ex, "Error starting checker '{CheckerName}'.", ForLog(checkerName)); return StatusCode(StatusCodes.Status500InternalServerError, "An unexpected error occurred."); } } @@ -208,7 +228,7 @@ public async Task StopChecker(string checkerName, CancellationTok var checkers = await pulsesScheduler.GetPulseCheckersAsync(cancellationToken).ConfigureAwait(false); if (!checkers.ContainsKey(checkerName)) { - logger?.LogWarning("Checker '{CheckerName}' not found for stopping.", checkerName); + logger?.LogWarning("Checker '{CheckerName}' not found for stopping.", ForLog(checkerName)); return NotFound($"Checker '{checkerName}' not found."); } @@ -217,7 +237,7 @@ public async Task StopChecker(string checkerName, CancellationTok } catch (Exception ex) { - logger?.LogError(ex, "Error stopping checker '{CheckerName}'.", checkerName); + logger?.LogError(ex, "Error stopping checker '{CheckerName}'.", ForLog(checkerName)); return StatusCode(StatusCodes.Status500InternalServerError, "An unexpected error occurred."); } } @@ -244,7 +264,7 @@ public async Task TriggerChecker(string checkerName, Cancellation var checkers = await pulsesScheduler.GetPulseCheckersAsync(cancellationToken).ConfigureAwait(false); if (!checkers.TryGetValue(checkerName, out var checker)) { - logger?.LogWarning("Checker '{CheckerName}' not found for triggering.", checkerName); + logger?.LogWarning("Checker '{CheckerName}' not found for triggering.", ForLog(checkerName)); return NotFound($"Checker '{checkerName}' not found."); } @@ -253,7 +273,7 @@ public async Task TriggerChecker(string checkerName, Cancellation } catch (Exception ex) { - logger?.LogError(ex, "Error triggering checker '{CheckerName}'.", checkerName); + logger?.LogError(ex, "Error triggering checker '{CheckerName}'.", ForLog(checkerName)); return StatusCode(StatusCodes.Status500InternalServerError, "An unexpected error occurred."); } } @@ -280,7 +300,7 @@ public async Task ResetChecker(string checkerName, CancellationTo var checkers = await pulsesScheduler.GetPulseCheckersAsync(cancellationToken).ConfigureAwait(false); if (!checkers.ContainsKey(checkerName)) { - logger?.LogWarning("Checker '{CheckerName}' not found for resetting.", checkerName); + logger?.LogWarning("Checker '{CheckerName}' not found for resetting.", ForLog(checkerName)); return NotFound($"Checker '{checkerName}' not found."); } @@ -290,7 +310,7 @@ public async Task ResetChecker(string checkerName, CancellationTo } catch (Exception ex) { - logger?.LogError(ex, "Error resetting checker '{CheckerName}'.", checkerName); + logger?.LogError(ex, "Error resetting checker '{CheckerName}'.", ForLog(checkerName)); return StatusCode(StatusCodes.Status500InternalServerError, "An unexpected error occurred."); } } diff --git a/src/Healthie.Api/Healthie.Api.csproj b/src/Healthie.Api/Healthie.Api.csproj index 414e0c3..583ee94 100644 --- a/src/Healthie.Api/Healthie.Api.csproj +++ b/src/Healthie.Api/Healthie.Api.csproj @@ -6,6 +6,10 @@ $(HealthieCommonTags);api;aspnetcore;rest + + + + diff --git a/src/Healthie.Dashboard/Components/HealthieDashboard.razor.cs b/src/Healthie.Dashboard/Components/HealthieDashboard.razor.cs index 4317b84afb38d159da8e8c7566b74d2d41f4baec..4e0815cfe6f3ab8aa36d3ee73162d0187a4939a6 100644 GIT binary patch delta 229 zcmXYru?Ye}5JeHeKoC2SZvkpxW?*1o1zNz~?dUGt&B)H4$3=EvVk3fum + public async Task UnsubscribeFromStateChangesAsync( + Func onStateChanged, + CancellationToken cancellationToken = default) + { + ArgumentNullException.ThrowIfNull(onStateChanged); + + await _handlersLock.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + _handlers.Remove(onStateChanged); + } + finally + { + _handlersLock.Release(); + } + } + /// /// Hands a state change to every subscriber. /// diff --git a/src/Healthie.Dashboard/Services/IHealthieDashboardService.cs b/src/Healthie.Dashboard/Services/IHealthieDashboardService.cs index cd7124d..7e44ec2 100644 --- a/src/Healthie.Dashboard/Services/IHealthieDashboardService.cs +++ b/src/Healthie.Dashboard/Services/IHealthieDashboardService.cs @@ -17,9 +17,31 @@ internal interface IHealthieDashboardService : IAsyncDisposable /// A token to monitor for cancellation requests. /// /// Subscribing is what starts the flow of updates, so this both registers the handler and - /// attaches to the checkers. Handlers are released when the service is disposed, which happens - /// when the circuit ends. + /// attaches to the checkers. Every subscription should be matched by + /// when the subscriber goes away. /// + /// + /// Stops handing state changes to a handler that was subscribed earlier. + /// + /// The handler passed to . + /// A token to monitor for cancellation requests. + /// + /// + /// This service is scoped to a circuit, so it was assumed that letting the circuit end released + /// everything. That holds only while the dashboard is mounted once per circuit, and it is not: + /// a host that routes to HealthieDashboard inside its own layout builds a new component + /// every time the user navigates back to it, on the same circuit. Without this, each of those + /// left its handler behind, and every later state change was handed to a component that had + /// been torn down. + /// + /// + /// Passing a handler that is not subscribed does nothing. + /// + /// + Task UnsubscribeFromStateChangesAsync( + Func onStateChanged, + CancellationToken cancellationToken = default); + Task SubscribeToStateChangesAsync( Func onStateChanged, CancellationToken cancellationToken = default); diff --git a/src/Healthie.Dashboard/StartupExtensions.cs b/src/Healthie.Dashboard/StartupExtensions.cs index c48d2a8..e3d7b25 100644 --- a/src/Healthie.Dashboard/StartupExtensions.cs +++ b/src/Healthie.Dashboard/StartupExtensions.cs @@ -3,6 +3,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; +using System.Net; namespace Healthie.Dashboard; @@ -63,7 +64,27 @@ public static IEndpointConventionBuilder MapHealthieUI( return endpoints.MapGet(DashboardPath, (HttpContext context) => { var opts = context.RequestServices.GetService(); - var title = opts?.DashboardTitle ?? "System Health"; + + context.Response.ContentType = "text/html"; + return context.Response.WriteAsync(BuildPage(opts?.DashboardTitle)); + }); + } + + /// + /// The host page that renders the dashboard component. + /// + /// + /// Separate from the endpoint so it can be asserted on without a host. This is the one place in + /// the library that builds HTML as a string instead of letting Razor build it, which is why the + /// title is encoded here by hand -- Razor would have done it everywhere else. + /// + internal static string BuildPage(string? dashboardTitle) + { + // The title is a host's setting, not a visitor's, so this matters only when a host + // builds it from something it did not write -- a per-tenant label, a value out of a + // database -- but that is a normal thing to do, and nothing here would have stopped it + // closing the title element and opening a script one. + var title = WebUtility.HtmlEncode(dashboardTitle ?? "System Health"); var html = $$""" @@ -89,8 +110,6 @@ embed the component in a page of their own and own their own body. """; - context.Response.ContentType = "text/html"; - return context.Response.WriteAsync(html); - }); + return html; } } diff --git a/src/Healthie.DependencyInjection/TimerPulseScheduler.cs b/src/Healthie.DependencyInjection/TimerPulseScheduler.cs index 14a13b8..4b51e34 100644 --- a/src/Healthie.DependencyInjection/TimerPulseScheduler.cs +++ b/src/Healthie.DependencyInjection/TimerPulseScheduler.cs @@ -24,6 +24,17 @@ public sealed class TimerPulseScheduler : IPulseScheduler, IAsyncDisposable, IDi private static readonly TimeSpan MaxDelay = TimeSpan.FromHours(1); private readonly ConcurrentDictionary _timers = new(); + + /// + /// Serialises scheduling, so a checker is never left running under a timer nobody holds. + /// + /// + /// The dictionary is concurrent, but installing a schedule is "stop the old one, then start the + /// new one" -- two operations, which a second caller can interleave with. A lock is the whole + /// fix: scheduling happens at startup and when somebody changes an interval, so it is never on + /// a hot path. + /// + private readonly SemaphoreSlim _scheduling = new(1, 1); private readonly ILogger? _logger; /// @@ -61,32 +72,66 @@ public async Task ScheduleAsync( // schedule should not be stopped by a request carrying a bad one. var cron = schedule.IsCron ? ParseCron(schedule.CronExpression!, checker.Name) : null; - await UnscheduleAsync(checker, cancellationToken).ConfigureAwait(false); + // Stopping the old schedule and installing the new one is two steps, and two callers + // scheduling one checker at once -- an interval changed from the dashboard while the + // scheduler starts it, say -- could each install a timer. Only the last would be in the + // dictionary; the other kept running with nothing able to reach it, and its linked + // CancellationTokenSource was never disposed, so its registration on the parent token + // outlived it too. + await _scheduling.WaitAsync(cancellationToken).ConfigureAwait(false); - var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - _timers[checker.Name] = cts; + try + { + StopExisting(checker.Name); - _ = Task.Run( - () => cron is null - ? RunPeriodicallyAsync(checker, schedule.Period!.Value, cts.Token) - : RunOnCronAsync(checker, cron, cts.Token), - cts.Token); + var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + _timers[checker.Name] = cts; + + _ = Task.Run( + () => cron is null + ? RunPeriodicallyAsync(checker, schedule.Period!.Value, cts.Token) + : RunOnCronAsync(checker, cron, cts.Token), + cts.Token); + } + finally + { + _scheduling.Release(); + } + } + + /// + /// Cancels and disposes the timer for a checker, if it has one. + /// + /// + /// Takes no lock, so it can be called from inside one. is the + /// same thing with the lock held. + /// + private void StopExisting(string name) + { + if (_timers.TryRemove(name, out var existing)) + { + existing.Cancel(); + existing.Dispose(); + } } /// - public Task UnscheduleAsync( + public async Task UnscheduleAsync( IPulseChecker checker, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(checker); - if (_timers.TryRemove(checker.Name, out var cts)) + await _scheduling.WaitAsync(cancellationToken).ConfigureAwait(false); + + try { - cts.Cancel(); - cts.Dispose(); + StopExisting(checker.Name); + } + finally + { + _scheduling.Release(); } - - return Task.CompletedTask; } /// @@ -235,6 +280,7 @@ public void Dispose() } _timers.Clear(); + _scheduling.Dispose(); } /// diff --git a/src/Healthie.Mcp/HealthieTools.cs b/src/Healthie.Mcp/HealthieTools.cs index bc238e3..0308489 100644 --- a/src/Healthie.Mcp/HealthieTools.cs +++ b/src/Healthie.Mcp/HealthieTools.cs @@ -16,8 +16,18 @@ namespace Healthie.Mcp; /// and are only exposed when they are turned on. /// [McpServerToolType] -public sealed class HealthieTools(IPulsesScheduler pulsesScheduler) +public sealed class HealthieTools(IPulsesScheduler pulsesScheduler, HealthieMcpOptions? options = null) { + /// + /// The host's options, or the defaults. + /// + /// + /// Optional so that adding it does not change how this type is constructed. DI always supplies + /// it -- AddHealthieMcp registers it -- so the fallback is for a caller that builds this + /// by hand, which used to be the only way it worked. + /// + private readonly HealthieMcpOptions _options = options ?? new HealthieMcpOptions(); + /// Returns the current health of every pulse checker. [McpServerTool(Name = "get_health_states")] [Description("Returns the current health of every monitored component, including the last result and when it last ran. Start here to see the overall health of the system.")] @@ -74,7 +84,10 @@ public async Task GetCheckHistoryAsync( // History is recorded oldest-first; a reader almost always wants the newest first. var newestFirst = history.AsEnumerable().Reverse().ToList(); - var page = newestFirst.Skip(Math.Max(offset, 0)).Take(Math.Clamp(limit, 1, 200)).ToList(); + // The host's cap, not a number hard-coded here. It was configurable and ignored, so a host + // that lowered it to keep responses small got no such thing. + var cap = Math.Max(_options.MaxHistoryPageSize, 1); + var page = newestFirst.Skip(Math.Max(offset, 0)).Take(Math.Clamp(limit, 1, cap)).ToList(); return new HistoryPage( name, diff --git a/src/Healthie.StateProviding.Relational/RelationalDialect.cs b/src/Healthie.StateProviding.Relational/RelationalDialect.cs index fc51118..0f5da4e 100644 --- a/src/Healthie.StateProviding.Relational/RelationalDialect.cs +++ b/src/Healthie.StateProviding.Relational/RelationalDialect.cs @@ -90,8 +90,15 @@ public sealed record RelationalDialect( /// /// A table name, optionally schema-qualified. Anything else is refused rather than interpolated. /// + /// + /// Ends at \z rather than $, which in .NET also matches immediately before a + /// single trailing newline -- so "state\n" passed a guard whose own error message says + /// it does not. A lone trailing newline is only whitespace to every engine here, so nothing + /// could be smuggled through it, but a guard that admits what it documents as impossible is + /// worth closing before something later depends on it. + /// private static readonly Regex SafeTableName = new( - @"^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$", + @"^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?\z", RegexOptions.CultureInvariant, TimeSpan.FromSeconds(1)); diff --git a/tests/Healthie.Tests.Unit/DashboardServiceTests.cs b/tests/Healthie.Tests.Unit/DashboardServiceTests.cs index bad0e79..953f4b6 100644 --- a/tests/Healthie.Tests.Unit/DashboardServiceTests.cs +++ b/tests/Healthie.Tests.Unit/DashboardServiceTests.cs @@ -29,6 +29,50 @@ private static (IHealthieDashboardService Service, ControllablePulseChecker Chec return (CreateService([checker, .. extra]), checker); } + /// + /// A handler stayed registered for the life of the circuit, and there was no way to drop one. + /// + /// + /// The service is scoped to a circuit, so it was assumed the circuit ending released everything. + /// That holds only while the dashboard is mounted once per circuit, and a host that routes to it + /// inside its own layout builds a new component every time the user navigates back -- each one + /// leaving its handler behind, still being called after it was torn down. + /// + [Fact] + public async Task UnsubscribeFromStateChangesAsync_StopsTheHandlerBeingCalled() + { + var (service, checker) = CreateServiceWithRealChecker(); + await using var _ = service; + + var calls = 0; + + Task Handler(string name, PulseCheckerState state) + { + Interlocked.Increment(ref calls); + return Task.CompletedTask; + } + + await service.SubscribeToStateChangesAsync(Handler, Ct); + await checker.TriggerAsync(Ct); + + var whileSubscribed = Volatile.Read(ref calls); + Assert.True(whileSubscribed > 0, "the handler should be called while it is subscribed"); + + await service.UnsubscribeFromStateChangesAsync(Handler, Ct); + await checker.TriggerAsync(Ct); + + Assert.Equal(whileSubscribed, Volatile.Read(ref calls)); + } + + [Fact] + public async Task UnsubscribeFromStateChangesAsync_ForAHandlerThatIsNotSubscribed_DoesNothing() + { + var (service, _) = CreateServiceWithRealChecker(); + await using var __ = service; + + await service.UnsubscribeFromStateChangesAsync((_, _) => Task.CompletedTask, Ct); + } + [Fact] public async Task SubscribeToStateChangesAsync_WhenACheckerChangesState_NotifiesTheSubscriber() { diff --git a/tests/Healthie.Tests.Unit/HardeningTests.cs b/tests/Healthie.Tests.Unit/HardeningTests.cs new file mode 100644 index 0000000..5c6aa1a --- /dev/null +++ b/tests/Healthie.Tests.Unit/HardeningTests.cs @@ -0,0 +1,98 @@ +using Healthie.Api.Controllers; +using Healthie.StateProviding.Relational; +using System.Text.RegularExpressions; + +namespace Healthie.Tests.Unit; + +/// +/// Guards that did not guard quite what they said they did. +/// +public class HardeningTests +{ + /// + /// The dashboard page is the one place in the library that builds HTML as a string instead of + /// letting Razor build it, and the title went in unencoded. + /// + [Fact] + public void ADashboardTitle_CannotCloseTheTitleElement() + { + var page = Healthie.Dashboard.StartupExtensions.BuildPage(""); + + // One closing title tag: the one the page itself writes. + Assert.Equal(1, Regex.Matches(page, "", RegexOptions.IgnoreCase).Count); + Assert.DoesNotContain("