From b80388ebefece834c3ef678944d4ac94f0ab8dbf Mon Sep 17 00:00:00 2001 From: Ivan Vydrin Date: Thu, 30 Jul 2026 05:07:29 +0300 Subject: [PATCH] Say so at startup when anyone can stop a checker without logging in AddHealthieController does not require authorization unless it is asked to, and the dashboard's AllowMutations defaults to true. Both are deliberate. The controller ships into someone else's MVC pipeline and cannot demand a policy it knows nothing about; a read-only dashboard would make every first run look broken. Neither default is wrong, and changing either breaks every application that maps them. What was missing was anyone being told. An application that maps one of these and stops there lets whoever can reach it stop a checker, slow its interval, or clear a failing streak -- which hides an incident rather than reporting one, from a tool whose whole job is to report them. That is worth a line in the log at startup. The check reads the endpoints, not the flags that built them. A host may have applied authorization some other way -- RequireAuthorization, an endpoint group, an attribute of its own -- and warning at an application that is already secured is how a warning gets filtered out, after which it is not there for the one that needs it. Only mutating methods are named, because reading health without authenticating is a choice an operator can reasonably make and listing it would drown the part that matters. Both shapes had to be handled, and the tests are why that is known: looking only for IAuthorizeData warned at AddHealthieController(requireAuthorization: true), because the convention adds an AuthorizeFilter, which is an IFilterMetadata and not an IAuthorizeData. The one configuration that had asked for authorization by name was the one being warned about. Healthie.Mcp is left alone: its AllowMutations defaults to false, and when it is off the tools that change anything are not registered at all. --- CHANGELOG.md | 12 ++ README.md | 4 +- src/Healthie.Api/Diagnostics/Log.cs | 28 +++ .../UnauthenticatedSurfaceWarning.cs | 92 +++++++++ src/Healthie.Api/StartupExtensions.cs | 8 + src/Healthie.Dashboard/Diagnostics/Log.cs | 26 +++ .../UnauthenticatedDashboardWarning.cs | 68 +++++++ src/Healthie.Dashboard/StartupExtensions.cs | 8 + .../UnauthenticatedSurfaceWarningTests.cs | 191 ++++++++++++++++++ 9 files changed, 436 insertions(+), 1 deletion(-) create mode 100644 src/Healthie.Api/Diagnostics/Log.cs create mode 100644 src/Healthie.Api/Diagnostics/UnauthenticatedSurfaceWarning.cs create mode 100644 src/Healthie.Dashboard/Diagnostics/Log.cs create mode 100644 src/Healthie.Dashboard/Diagnostics/UnauthenticatedDashboardWarning.cs create mode 100644 tests/Healthie.Tests.Unit/UnauthenticatedSurfaceWarningTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 80fdb7d..10f1de6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,18 @@ its code behaves exactly as it did. to completion without interleaving anything else, which is the guarantee a read-then-write cannot give. Durability is whatever the Redis is configured for, and the package README says so rather than implying more. +- **A startup warning when a surface that can change a checker is reachable without + authenticating.** `AddHealthieController` still does not require authorization unless asked, and + the dashboard's `AllowMutations` still defaults to `true` -- both are deliberate and changing + either would break every application that maps them. What was missing was anyone being told: an + application that maps one and stops there lets whoever can reach it stop a checker or clear a + failing streak, which hides an incident rather than reporting one. Logged once, at `Warning`, on + application start. + + Asked of the endpoints rather than of the flags that built them, so an application that applied + authorization its own way -- `RequireAuthorization()`, an endpoint group, its own attribute -- is + not warned at. A warning that fires on correctly secured applications gets filtered out, and then + it is not there for the one that needs it. - **Schedules.** `PulseSchedule` says either "every this long" or "on this cron expression", and sits alongside `PulseInterval` rather than replacing it. The enum stopped at five minutes, which is short of what a certificate-expiry or disk-space check wants. Cron is standard Unix syntax and diff --git a/README.md b/README.md index dc43153..2af2d89 100644 --- a/README.md +++ b/README.md @@ -834,7 +834,9 @@ and both are deliberate as they stand: - **`Healthie.Api` requires no authorization unless the host asks for it**, and the dashboard's `HealthieUIOptions.AllowMutations` defaults to `true`. A host that maps either and does nothing else exposes read *and* write control of its checkers. Both are documented, and changing either - default is a behaviour break for every existing consumer. + default is a behaviour break for every existing consumer -- so instead of changing them, both now + log a `Warning` at startup naming exactly what is exposed. The warning reads the endpoints, not + the flags, so securing them any way at all silences it. - **Restore is not pinned by hash.** The fix is NuGet lock files, which means every package change needs the lock updated and CI running in locked mode. diff --git a/src/Healthie.Api/Diagnostics/Log.cs b/src/Healthie.Api/Diagnostics/Log.cs new file mode 100644 index 0000000..5bbfba2 --- /dev/null +++ b/src/Healthie.Api/Diagnostics/Log.cs @@ -0,0 +1,28 @@ +using Microsoft.Extensions.Logging; + +namespace Healthie.Api.Diagnostics; + +/// +/// The log messages writes. +/// +/// +/// Source-generated, as in the other packages, and with event ids in their own 4000 range so a +/// filter written against them keeps meaning what it meant. +/// +internal static partial class Log +{ + /// + /// Warning, and only once at startup. It describes a configuration an operator chose and can + /// change, not something going wrong at runtime, so repeating it per request would bury the + /// logs it is trying to be noticed in. + /// + [LoggerMessage( + EventId = 4001, + Level = LogLevel.Warning, + Message = "Healthie: {Count} endpoint(s) that can change a pulse checker are reachable " + + "without authenticating -- {Routes}. Anyone who can reach this application can stop a " + + "checker or clear a failing streak, which hides an incident rather than reporting one. " + + "Pass requireAuthorization: true to AddHealthieController, or apply your own " + + "authorization to these endpoints.")] + public static partial void MutatingEndpointsAreUnauthenticated(ILogger logger, int count, string routes); +} diff --git a/src/Healthie.Api/Diagnostics/UnauthenticatedSurfaceWarning.cs b/src/Healthie.Api/Diagnostics/UnauthenticatedSurfaceWarning.cs new file mode 100644 index 0000000..eb6c21f --- /dev/null +++ b/src/Healthie.Api/Diagnostics/UnauthenticatedSurfaceWarning.cs @@ -0,0 +1,92 @@ +using Healthie.Api.Routes; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Authorization; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace Healthie.Api.Diagnostics; + +/// +/// Says so, once and loudly, when the endpoints that can change a checker are reachable without +/// authenticating. +/// +/// +/// +/// AddHealthieController does not require authorization unless it is asked to, which is a +/// deliberate default -- this controller ships into someone else's MVC pipeline, and demanding a +/// policy it knows nothing about would break every application that maps it. The consequence is +/// that an application which maps it and does nothing else lets anyone stop a checker or clear a +/// failing streak. That is not a wrong default so much as one worth being told about. +/// +/// +/// Asked of the endpoints rather than of the flag that built them: the host may have applied +/// authorization some other way -- a group, middleware, an +/// of its own -- and a warning that cried wolf at a correctly +/// secured application would be scrolled past within a week, which costs more than saying nothing. +/// +/// +internal sealed class UnauthenticatedSurfaceWarning( + EndpointDataSource endpoints, + IHostApplicationLifetime lifetime, + ILogger logger) : IHostedService +{ + /// The methods that change something, as opposed to reporting it. + private static readonly string[] MutatingMethods = ["POST", "PUT", "PATCH", "DELETE"]; + + public Task StartAsync(CancellationToken cancellationToken) + { + // After the application has started, because that is when the endpoints exist. Reading them + // during StartAsync races the routing system that builds them. + lifetime.ApplicationStarted.Register(Warn); + + return Task.CompletedTask; + } + + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + private void Warn() + { + var unprotected = endpoints.Endpoints + .OfType() + .Where(IsHealthieRoute) + .Where(Mutates) + .Where(endpoint => !IsProtected(endpoint)) + .Select(endpoint => endpoint.RoutePattern.RawText) + .Where(route => route is not null) + .Distinct(StringComparer.OrdinalIgnoreCase) + .Order(StringComparer.OrdinalIgnoreCase) + .ToArray(); + + if (unprotected.Length == 0) + { + return; + } + + Log.MutatingEndpointsAreUnauthenticated(logger, unprotected.Length, string.Join(", ", unprotected!)); + } + + /// + /// Whether anything on this endpoint requires an authenticated caller. + /// + /// + /// Two shapes, because authorization arrives two ways and only one of them is metadata. + /// RequireAuthorization() and [Authorize] put an on + /// the endpoint. AddHealthieController(requireAuthorization: true) adds an + /// through an MVC convention, and that is an + /// IFilterMetadata rather than an -- so looking only for the + /// latter warned about the one configuration that had asked for authorization by name. + /// + private static bool IsProtected(Endpoint endpoint) => + endpoint.Metadata.GetMetadata() is not null + || endpoint.Metadata.GetMetadata() is not null; + + private static bool IsHealthieRoute(RouteEndpoint endpoint) => + endpoint.RoutePattern.RawText?.StartsWith(RoutesConstants.HealthieApiRoute, StringComparison.OrdinalIgnoreCase) + ?? false; + + private static bool Mutates(RouteEndpoint endpoint) => + endpoint.Metadata.GetMetadata() is { } methods + && methods.HttpMethods.Any(method => MutatingMethods.Contains(method, StringComparer.OrdinalIgnoreCase)); +} diff --git a/src/Healthie.Api/StartupExtensions.cs b/src/Healthie.Api/StartupExtensions.cs index 0172e27..880a9fb 100644 --- a/src/Healthie.Api/StartupExtensions.cs +++ b/src/Healthie.Api/StartupExtensions.cs @@ -1,6 +1,9 @@ using Healthie.Api.Controllers; using Healthie.Api.Conventions; +using Healthie.Api.Diagnostics; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Hosting; namespace Healthie.Api; @@ -37,6 +40,11 @@ public static IMvcBuilder AddHealthieController( // Ensure HealthCheckersController from Healthie.Api assembly is discovered. mvcBuilder.AddApplicationPart(typeof(HealthCheckersController).Assembly); + // Says so at startup if the endpoints that can change a checker end up reachable without + // authenticating. TryAdd because calling this twice should not warn twice. + services.TryAddEnumerable( + ServiceDescriptor.Singleton()); + return mvcBuilder; } } diff --git a/src/Healthie.Dashboard/Diagnostics/Log.cs b/src/Healthie.Dashboard/Diagnostics/Log.cs new file mode 100644 index 0000000..82f8be5 --- /dev/null +++ b/src/Healthie.Dashboard/Diagnostics/Log.cs @@ -0,0 +1,26 @@ +using Microsoft.Extensions.Logging; + +namespace Healthie.Dashboard.Diagnostics; + +/// +/// The log messages the dashboard writes. +/// +/// +/// Source-generated, as in the other packages, and with event ids in their own 5000 range so a +/// filter written against them keeps meaning what it meant. +/// +internal static partial class Log +{ + /// + /// Warning, and only once at startup. It describes a configuration an operator chose and can + /// change, not something going wrong at runtime. + /// + [LoggerMessage( + EventId = 5001, + Level = LogLevel.Warning, + Message = "Healthie: the dashboard at {Path} is reachable without authenticating and its " + + "controls are on, so anyone who can reach this application can pause a checker or reset " + + "a failing streak. Chain RequireAuthorization() onto MapHealthieUI(), or set " + + "HealthieUIOptions.AllowMutations to false to serve it read-only.")] + public static partial void DashboardIsUnauthenticatedAndWritable(ILogger logger, string path); +} diff --git a/src/Healthie.Dashboard/Diagnostics/UnauthenticatedDashboardWarning.cs b/src/Healthie.Dashboard/Diagnostics/UnauthenticatedDashboardWarning.cs new file mode 100644 index 0000000..786c697 --- /dev/null +++ b/src/Healthie.Dashboard/Diagnostics/UnauthenticatedDashboardWarning.cs @@ -0,0 +1,68 @@ +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace Healthie.Dashboard.Diagnostics; + +/// +/// Says so, once and loudly, when the dashboard is reachable without authenticating and its +/// controls are on. +/// +/// +/// +/// defaults to true because the dashboard +/// exists to manage checkers and a read-only default would make every first run look broken. It is +/// not authorization and does not pretend to be: it decides which controls are rendered, for +/// everyone, and RequireAuthorization on the mapped endpoint is the other half. An +/// application that maps the dashboard and stops there has given anyone who can reach it the +/// ability to pause a checker or reset a failing streak. +/// +/// +/// Asked of the endpoint rather than assumed from the option, so an application that secured it -- +/// by chaining RequireAuthorization, by an endpoint group, by its own attribute -- is not +/// warned at. A warning that fires on correctly secured applications gets filtered out, and then it +/// is not there for the one that needs it. +/// +/// +internal sealed class UnauthenticatedDashboardWarning( + EndpointDataSource endpoints, + HealthieUIOptions options, + IHostApplicationLifetime lifetime, + ILogger logger) : IHostedService +{ + public Task StartAsync(CancellationToken cancellationToken) + { + // Nothing to say when the controls are not rendered: reaching the board then shows health + // and changes nothing. + if (!options.AllowMutations) + { + return Task.CompletedTask; + } + + // After the application has started, because that is when the endpoints exist. + lifetime.ApplicationStarted.Register(Warn); + + return Task.CompletedTask; + } + + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + private void Warn() + { + var dashboard = endpoints.Endpoints + .OfType() + .FirstOrDefault(endpoint => string.Equals( + "/" + endpoint.RoutePattern.RawText?.TrimStart('/'), + StartupExtensions.DashboardPath, + StringComparison.OrdinalIgnoreCase)); + + // Not mapped at all, or already behind an authorization policy. + if (dashboard is null || dashboard.Metadata.GetMetadata() is not null) + { + return; + } + + Log.DashboardIsUnauthenticatedAndWritable(logger, StartupExtensions.DashboardPath); + } +} diff --git a/src/Healthie.Dashboard/StartupExtensions.cs b/src/Healthie.Dashboard/StartupExtensions.cs index e3d7b25..a4431de 100644 --- a/src/Healthie.Dashboard/StartupExtensions.cs +++ b/src/Healthie.Dashboard/StartupExtensions.cs @@ -1,8 +1,11 @@ +using Healthie.Dashboard.Diagnostics; using Healthie.Dashboard.Services; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Hosting; using System.Net; namespace Healthie.Dashboard; @@ -42,6 +45,11 @@ public static IServiceCollection AddHealthieUI( services.AddScoped(); services.AddScoped(); + // Says so at startup if the board ends up reachable without authenticating while its + // controls are on. TryAdd because calling this twice should not warn twice. + services.TryAddEnumerable( + ServiceDescriptor.Singleton()); + return services; } diff --git a/tests/Healthie.Tests.Unit/UnauthenticatedSurfaceWarningTests.cs b/tests/Healthie.Tests.Unit/UnauthenticatedSurfaceWarningTests.cs new file mode 100644 index 0000000..111865f --- /dev/null +++ b/tests/Healthie.Tests.Unit/UnauthenticatedSurfaceWarningTests.cs @@ -0,0 +1,191 @@ +using Healthie.Abstractions.Scheduling; +using Healthie.Api; +using Healthie.Dashboard; +using Healthie.DependencyInjection; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace Healthie.Tests.Unit; + +/// +/// The startup warning for surfaces that can change a checker without anyone authenticating. +/// +/// +/// Driven through a real host so the endpoints exist and carry the metadata the warning reads. +/// Asserting on the option instead would only restate the code: the point is that the check follows +/// what was actually applied, including authorization the host added its own way. +/// +public class UnauthenticatedSurfaceWarningTests +{ + /// Captures what was logged, so a warning can be asserted on rather than eyeballed. + private sealed class CapturingProvider : ILoggerProvider + { + private readonly List<(LogLevel Level, string Message)> _entries = []; + + public IReadOnlyList<(LogLevel Level, string Message)> Entries + { + get + { + lock (_entries) + { + return [.. _entries]; + } + } + } + + public ILogger CreateLogger(string categoryName) => new Capturing(this); + + public void Dispose() + { + } + + private void Add(LogLevel level, string message) + { + lock (_entries) + { + _entries.Add((level, message)); + } + } + + private sealed class Capturing(CapturingProvider owner) : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) => + owner.Add(logLevel, formatter(state, exception)); + } + } + + private static async Task> RunAsync( + Action configureServices, + Action configureApp) + { + var capture = new CapturingProvider(); + + var builder = WebApplication.CreateBuilder(); + // Port 0 lets the OS pick a free one, so parallel tests cannot collide and no + // TestHost package is needed to get real endpoints built. + builder.WebHost.UseUrls("http://127.0.0.1:0"); + builder.Logging.ClearProviders(); + builder.Logging.AddProvider(capture); + + builder.Services.AddHealthie(typeof(UnauthenticatedSurfaceWarningTests).Assembly); + builder.Services.AddAuthorization(); + configureServices(builder.Services); + + var app = builder.Build(); + configureApp(app); + + await app.StartAsync(); + await app.StopAsync(); + await app.DisposeAsync(); + + return capture.Entries; + } + + private static bool Warned(IReadOnlyList<(LogLevel Level, string Message)> entries, string fragment) => + entries.Any(e => e.Level == LogLevel.Warning && e.Message.Contains(fragment, StringComparison.Ordinal)); + + [Fact] + public async Task AnUngatedApi_WarnsAboutTheEndpointsThatCanChangeAChecker() + { + var entries = await RunAsync( + services => services.AddHealthieController(), + app => app.MapControllers()); + + Assert.True( + Warned(entries, "can change a pulse checker are reachable"), + "mapping the controller with no authorization should have warned"); + } + + /// + /// The warning follows what was applied, not the flag that was passed, so a host that required + /// authorization must not be warned at. + /// + [Fact] + public async Task AnApiThatRequiresAuthorization_IsNotWarnedAt() + { + var entries = await RunAsync( + services => services.AddHealthieController(requireAuthorization: true), + app => app.MapControllers()); + + Assert.False(Warned(entries, "can change a pulse checker are reachable")); + } + + /// + /// And a host that secured the endpoints its own way -- not through the flag -- is also not + /// warned at. This is the case that makes the check worth doing over the endpoints. + /// + [Fact] + public async Task AnApiSecuredByTheHostItself_IsNotWarnedAt() + { + var entries = await RunAsync( + services => services.AddHealthieController(), + app => app.MapControllers().RequireAuthorization()); + + Assert.False(Warned(entries, "can change a pulse checker are reachable")); + } + + [Fact] + public async Task AnUngatedDashboardWithControlsOn_Warns() + { + var entries = await RunAsync( + services => services.AddHealthieUI(), + app => app.MapHealthieUI()); + + Assert.True( + Warned(entries, "controls are on"), + "mapping a writable dashboard with no authorization should have warned"); + } + + [Fact] + public async Task AReadOnlyDashboard_IsNotWarnedAt() + { + var entries = await RunAsync( + services => services.AddHealthieUI(options => options.AllowMutations = false), + app => app.MapHealthieUI()); + + Assert.False(Warned(entries, "controls are on")); + } + + [Fact] + public async Task ADashboardBehindAuthorization_IsNotWarnedAt() + { + var entries = await RunAsync( + services => services.AddHealthieUI(), + app => app.MapHealthieUI().RequireAuthorization()); + + Assert.False(Warned(entries, "controls are on")); + } + + /// + /// The reads are not the concern: seeing health without authenticating is a choice an operator + /// can reasonably make, and warning about it would drown the case that matters. + /// + [Fact] + public async Task TheWarning_NamesOnlyTheMutatingRoutes() + { + var entries = await RunAsync( + services => services.AddHealthieController(), + app => app.MapControllers()); + + var warning = entries.Single(e => + e.Level == LogLevel.Warning && e.Message.Contains("can change a pulse checker", StringComparison.Ordinal)); + + Assert.Contains("trigger", warning.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("reset", warning.Message, StringComparison.OrdinalIgnoreCase); + + // "intervals" is the read-only listing endpoint. + Assert.DoesNotContain("healthie/intervals", warning.Message, StringComparison.OrdinalIgnoreCase); + } +}