Close what a leak-and-security sweep found

.github/workflows/publish.yml

@@ -20,11 +20,11 @@ on:
     tags:
       - 'v*'
 
+# Nothing at the top level, so a job added later starts with no write access and has to ask for
+# what it needs. Scorecard rates a workflow-wide `contents: write` a high finding for exactly that
+# reason: it is inherited by every job, including ones nobody thought about when it was granted.
 permissions:
-  contents: write
-  # Lets this workflow ask GitHub for the signed OIDC token that NuGet.org trades for a
-  # short-lived publishing key. Without it the login step below cannot get a token at all.
-  id-token: write
+  contents: read
 
 env:
   CONFIGURATION: Release
@@ -35,6 +35,13 @@ jobs:
   release:
     runs-on: ubuntu-latest
 
+    permissions:
+      # Creating the GitHub release at the end of this job.
+      contents: write
+      # Lets this job ask GitHub for the signed OIDC token that NuGet.org trades for a short-lived
+      # publishing key. Without it the login step below cannot get a token at all.
+      id-token: write
+
     steps:
       - name: Checkout
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7

CHANGELOG.md

@@ -11,6 +11,34 @@ 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.
+- **The release workflow granted `contents: write` to every job in it.** Only the one job that
+  creates the GitHub release needs it, and the top level now grants nothing but read, so a job added
+  later starts with no write access rather than inheriting it.
+- **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

src/Healthie.Api/Controllers/HealthCheckersController.cs

@@ -99,7 +99,7 @@
             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 +108,7 @@
         }
         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 +136,7 @@
             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 +145,7 @@
         }
         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 +172,7 @@
             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 +181,7 @@
         }
         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 +208,7 @@
             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 +217,7 @@
         }
         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 +244,7 @@
             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 +253,7 @@
         }
         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 +280,7 @@
             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,8 +290,31 @@
         }
         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.");
         }
     }
+
+    /// <summary>
+    /// Strips control characters from a name before it is written to a log.
+    /// </summary>
+    /// <param name="name">The name as the route supplied it.</param>
+    /// <returns>The name, with any control character replaced.</returns>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    internal static string ForLog(string name)
+    {
+        if (!name.Any(char.IsControl))
+        {
+            return name;
+        }
+
+        return new string([.. name.Select(c => char.IsControl(c) ? '\uFFFD' : c)]);
+    }
+
 }

src/Healthie.Api/Healthie.Api.csproj

@@ -6,6 +6,10 @@
     <PackageTags>$(HealthieCommonTags);api;aspnetcore;rest</PackageTags>
   </PropertyGroup>
 
+  <ItemGroup>
+    <InternalsVisibleTo Include="Healthie.Tests.Unit" />
+  </ItemGroup>
+
   <ItemGroup>
     <FrameworkReference Include="Microsoft.AspNetCore.App" />
   </ItemGroup>

src/Healthie.Dashboard/Services/HealthieDashboardService.cs

@@ -321,6 +321,24 @@ public async Task SubscribeToStateChangesAsync(
         }
     }
 
+    /// <inheritdoc />
+    public async Task UnsubscribeFromStateChangesAsync(
+        Func<string, PulseCheckerState, Task> onStateChanged,
+        CancellationToken cancellationToken = default)
+    {
+        ArgumentNullException.ThrowIfNull(onStateChanged);
+
+        await _handlersLock.WaitAsync(cancellationToken).ConfigureAwait(false);
+        try
+        {
+            _handlers.Remove(onStateChanged);
+        }
+        finally
+        {
+            _handlersLock.Release();
+        }
+    }
+
     /// <summary>
     /// Hands a state change to every subscriber.
     /// </summary>
@@ -330,7 +348,22 @@ public async Task SubscribeToStateChangesAsync(
     /// </remarks>
     private async Task NotifyAsync(string name, PulseCheckerState state)
     {
-        foreach (var handler in _handlers.ToArray())
+        // Copied under the lock: this runs on whichever thread ran the check, while a component
+        // mounting or being disposed may be writing the same list.
+        await _handlersLock.WaitAsync().ConfigureAwait(false);
+
+        Func<string, PulseCheckerState, Task>[] handlers;
+
+        try
+        {
+            handlers = [.. _handlers];
+        }
+        finally
+        {
+            _handlersLock.Release();
+        }
+
+        foreach (var handler in handlers)
         {
             try
             {

src/Healthie.Dashboard/Services/IHealthieDashboardService.cs

@@ -17,13 +17,35 @@ internal interface IHealthieDashboardService : IAsyncDisposable
     /// <param name="cancellationToken">A token to monitor for cancellation requests.</param>
     /// <remarks>
     /// 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
+    /// <see cref="UnsubscribeFromStateChangesAsync"/> when the subscriber goes away.
     /// </remarks>
     Task SubscribeToStateChangesAsync(
         Func<string, PulseCheckerState, Task> onStateChanged,
         CancellationToken cancellationToken = default);
 
+    /// <summary>
+    /// Stops handing state changes to a handler that was subscribed earlier.
+    /// </summary>
+    /// <param name="onStateChanged">The handler passed to <see cref="SubscribeToStateChangesAsync"/>.</param>
+    /// <param name="cancellationToken">A token to monitor for cancellation requests.</param>
+    /// <remarks>
+    /// <para>
+    /// 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 <c>HealthieDashboard</c> 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.
+    /// </para>
+    /// <para>
+    /// Passing a handler that is not subscribed does nothing.
+    /// </para>
+    /// </remarks>
+    Task UnsubscribeFromStateChangesAsync(
+        Func<string, PulseCheckerState, Task> onStateChanged,
+        CancellationToken cancellationToken = default);
+
     /// <summary>
     /// Triggers every registered pulse checker.
     /// </summary>

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<HealthieUIOptions>();
-            var title = opts?.DashboardTitle ?? "System Health";
+
+            context.Response.ContentType = "text/html";
+            return context.Response.WriteAsync(BuildPage(opts?.DashboardTitle));
+        });
+    }
+
+    /// <summary>
+    /// The host page that renders the dashboard component.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    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 = $$"""
                 <!DOCTYPE html>
@@ -89,8 +110,6 @@ embed the component in a page of their own and own their own body.
                 </html>
                 """;
 
-            context.Response.ContentType = "text/html";
-            return context.Response.WriteAsync(html);
-        });
+            return html;
     }
 }