Operability: readiness probe, correlated failures, a loud proxy check, and real pagination

src/WidgetWorks.WebApi/Diagnostics/CorrelationId.cs

@@ -0,0 +1,65 @@
+namespace WidgetWorks.WebApi.Diagnostics;
+
+/// <summary>
+/// Gives every request an identifier the caller can quote back.
+///
+/// Without one, a customer saying "checkout failed around three" is the entire incident report:
+/// the response carried nothing, and the log line for their exception sits among every other line
+/// from that minute with no way to tell them apart. With one, six characters turn diagnosis into
+/// a lookup.
+/// </summary>
+public static class CorrelationId
+{
+    /// <summary>Header the id is read from and echoed on, matching the de-facto convention.</summary>
+    public const string HeaderName = "X-Correlation-Id";
+
+    /// <summary>Longest inbound id accepted, so a caller cannot push arbitrary text into the logs.</summary>
+    private const int MaxLength = 64;
+
+    /// <summary>
+    /// Resolves the id for a request: an inbound one when the caller supplied something sane, so a
+    /// trace already begun upstream keeps its thread, otherwise the id ASP.NET already assigns.
+    ///
+    /// Inbound values are sanitised rather than trusted. This string ends up in log messages, and
+    /// text carrying newlines could otherwise forge whole log entries (CWE-117) — the same class of
+    /// defect already fixed once in this codebase, on the order-status path.
+    /// </summary>
+    public static string Resolve(HttpContext context)
+    {
+        ArgumentNullException.ThrowIfNull(context);
+
+        var supplied = context.Request.Headers[HeaderName].ToString();
+        if (!string.IsNullOrWhiteSpace(supplied))
+        {
+            var cleaned = Sanitize(supplied);
+            if (cleaned.Length > 0)
+            {
+                return cleaned;
+            }
+        }
+
+        return context.TraceIdentifier;
+    }
+
+    /// <summary>Keeps letters, digits and a few separators; drops everything else and truncates.</summary>
+    private static string Sanitize(string value)
+    {
+        var kept = new char[Math.Min(value.Length, MaxLength)];
+        var length = 0;
+
+        foreach (var c in value)
+        {
+            if (length == kept.Length)
+            {
+                break;
+            }
+
+            if (char.IsAsciiLetterOrDigit(c) || c is '-' or '_' or ':' or '.')
+            {
+                kept[length++] = c;
+            }
+        }
+
+        return new string(kept, 0, length);
+    }
+}

src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs

@@ -0,0 +1,61 @@
+using Microsoft.AspNetCore.Diagnostics;
+
+namespace WidgetWorks.WebApi.Diagnostics;
+
+public static class ExceptionHandling
+{
+    /// <summary>
+    /// Turns an unhandled exception into a supportable answer.
+    ///
+    /// The previous behaviour was safe but opaque: no handler meant a bare 500 with an empty body,
+    /// and while that leaks nothing — the developer exception page is Development-only — it left
+    /// nothing connecting what the customer saw to what the logs recorded.
+    ///
+    /// The response now carries a correlation id and the same id is on the log line, so a report
+    /// becomes a lookup. The body still says nothing about the failure itself: an exception message
+    /// can name a host, a column, or a connection string, and this reaches anonymous callers.
+    /// </summary>
+    public static void UseWidgetWorksExceptionHandler(this WebApplication app)
+    {
+        app.UseExceptionHandler(builder => builder.Run(async context =>
+        {
+            var correlationId = CorrelationId.Resolve(context);
+            var feature = context.Features.Get<IExceptionHandlerFeature>();
+
+            // The path is sanitised because Request.Path.Value is the *decoded* path: a URL
+            // containing %0A arrives here as a real newline, which would end this log entry and
+            // begin one the caller wrote (CWE-117). The correlation id is already clean by
+            // construction; the method comes from the server's own parser.
+            app.Logger.LogError(
+                feature?.Error,
+                "Unhandled exception for {Method} {Path} (correlation {CorrelationId}).",
+                LogSafe.Text(context.Request.Method, maxLength: 16),
+                LogSafe.Text(context.Request.Path.Value),
+                correlationId);
+
+            context.Response.StatusCode = StatusCodes.Status500InternalServerError;
+            context.Response.Headers[CorrelationId.HeaderName] = correlationId;
+
+            await context.Response.WriteAsJsonAsync(new
+            {
+                error = "Something went wrong on our side. Quote the reference below if you contact us.",
+                correlationId,
+            });
+        }));
+
+        // Echoed on every response, not only failures, so a caller can correlate a slow or wrong
+        // answer as readily as a failed one — and so a support conversation can start before anyone
+        // has looked at a log.
+        app.Use(async (context, next) =>
+        {
+            var correlationId = CorrelationId.Resolve(context);
+            context.Response.OnStarting(() =>
+            {
+                context.Response.Headers[CorrelationId.HeaderName] = correlationId;
+                return Task.CompletedTask;
+            });
+
+            await next(context);
+        });
+    }
+}

src/WidgetWorks.WebApi/Diagnostics/HealthEndpoints.cs

@@ -0,0 +1,87 @@
+using System.Data;
+using Dapper;
+using WidgetWorks.Infrastructure.Persistence;
+
+namespace WidgetWorks.WebApi.Diagnostics;
+
+/// <summary>
+/// Two probes answering two different questions, because conflating them is how a monitoring signal
+/// ends up unable to report bad news.
+/// </summary>
+public static class HealthEndpoints
+{
+    /// <param name="migrationSucceeded">Outcome of the startup migration — a fact about this boot.</param>
+    /// <param name="migrationError">Why it failed, when it did.</param>
+    public static void MapHealthEndpoints(this IEndpointRouteBuilder routes, bool migrationSucceeded, string? migrationError)
+    {
+        // Liveness. Answers "did this process start correctly", which is a fact settled at boot and
+        // needs no database. Deliberately unchanged in contract: the provisioning script watches it
+        // on first boot and stops the app when it reports unhealthy, and the keep-warm schedule
+        // pings it every few minutes to hold a free-tier instance loaded.
+        //
+        // That ping is why this must not touch the database. Waking a serverless database on every
+        // warm-up would hold a metered resource awake around the clock — roughly 180 CU-hrs against
+        // a 100 CU-hr monthly budget. Warm the app, let the database sleep.
+        routes.MapGet("/health", (TimeProvider clock) => migrationSucceeded
+            ? Results.Ok(new { status = "ok", utcNow = clock.GetUtcNow() })
+            : Results.Json(
+                new { status = "unhealthy", reason = "database migration failed", detail = migrationError, utcNow = clock.GetUtcNow() },
+                statusCode: StatusCodes.Status503ServiceUnavailable));
+
+        // Readiness. Answers "can this instance serve a request right now", which liveness cannot:
+        // a process that started perfectly is still useless once its database goes away, and the
+        // startup answer never changes to say so.
+        //
+        // Point platform probes and alerting here, not at /health — and keep scheduled warm-up
+        // pings off it, or the cost the shallow probe exists to avoid comes straight back.
+        routes.MapGet("/health/ready", async (
+            IDbConnectionFactory connections,
+            TimeProvider clock,
+            ILoggerFactory loggerFactory,
+            CancellationToken ct) =>
+        {
+            var startedAt = clock.GetUtcNow();
+            try
+            {
+                using var db = await connections.OpenAsync(ct);
+                await db.ExecuteScalarAsync<int>(new CommandDefinition("select 1", cancellationToken: ct));
+
+                return Results.Ok(new
+                {
+                    status = "ready",
+                    database = "ok",
+                    migrationSucceeded,
+                    checkedAt = startedAt,
+                });
+            }
+            catch (OperationCanceledException) when (ct.IsCancellationRequested)
+            {
+                // The caller gave up or the host is shutting down. Not a verdict about the database,
+                // so it is reported as unavailable without being logged as a fault.
+                return Results.Json(
+                    new { status = "unavailable", reason = "the readiness check was cancelled", checkedAt = startedAt },
+                    statusCode: StatusCodes.Status503ServiceUnavailable);
+            }
+            catch (Exception ex)
+            {
+                // Logged rather than swallowed: the probe's 503 tells the platform to stop sending
+                // traffic here, and this line is the only place the reason survives.
+                loggerFactory
+                    .CreateLogger(typeof(HealthEndpoints))
+                    .LogError(ex, "Readiness check failed: the database did not answer.");
+
+                return Results.Json(
+                    new
+                    {
+                        status = "not ready",
+                        database = "unreachable",
+                        // The exception type, never its message: a connection failure can carry a
+                        // host name or a user, and this endpoint is unauthenticated.
+                        reason = ex.GetType().Name,
+                        checkedAt = startedAt,
+                    },
+                    statusCode: StatusCodes.Status503ServiceUnavailable);
+            }
+        });
+    }
+}

src/WidgetWorks.WebApi/Diagnostics/LogSafe.cs

@@ -0,0 +1,48 @@
+namespace WidgetWorks.WebApi.Diagnostics;
+
+/// <summary>
+/// Makes caller-controlled text safe to put in a log message.
+///
+/// A log file is a flat stream of lines, so any value carrying a newline can end one entry and
+/// begin another that reads exactly like a genuine record — an attacker choosing what the log
+/// appears to say about them (CWE-117). Request paths are the easy example: <c>Path.Value</c> is
+/// the *decoded* path, so <c>%0A</c> in a URL arrives as a real line break.
+/// </summary>
+public static class LogSafe
+{
+    /// <summary>Shown in place of a value that sanitised away to nothing, so the field is never blank.</summary>
+    public const string Empty = "(empty)";
+
+    /// <summary>
+    /// Strips control characters and truncates. Everything printable is kept, because the point is
+    /// to preserve what was actually requested — a path full of odd characters is exactly what an
+    /// operator needs to see — while removing the ones that can restructure the log itself.
+    /// </summary>
+    public static string Text(string? value, int maxLength = 256)
+    {
+        if (string.IsNullOrEmpty(value))
+        {
+            return Empty;
+        }
+
+        var kept = new char[Math.Min(value.Length, maxLength)];
+        var length = 0;
+
+        foreach (var c in value)
+        {
+            if (length == kept.Length)
+            {
+                break;
+            }
+
+            // char.IsControl covers CR, LF, tab and the rest of the C0/C1 ranges — the characters
+            // that can break a line or confuse a log viewer.
+            if (!char.IsControl(c))
+            {
+                kept[length++] = c;
+            }
+        }
+
+        return length == 0 ? Empty : new string(kept, 0, length);
+    }
+}

src/WidgetWorks.WebApi/Program.cs

@@ -18,6 +18,7 @@
 using WidgetWorks.WebApi.Payments;
 using WidgetWorks.WebApi.Security;
 using WidgetWorks.Application.Checkout.ReleaseStale;
+using WidgetWorks.WebApi.Diagnostics;
 using WidgetWorks.WebApi.Hosting;
 using WidgetWorks.WebApi.RateLimiting;
 using WidgetWorks.WebApi.TwoFactor;
@@ -28,6 +29,7 @@
 builder.Services.AddInfrastructure(builder.Configuration);
 builder.Services.AddOpenApi();
 builder.Services.AddWidgetWorksRateLimiting(builder.Configuration);
+builder.Services.AddSingleton<ProxyConfigurationCheck>();
 
 // Stock held by an order whose payment never settles is returned to sale on a timer. Options are
 // bound here so the sweep can be retuned, or turned off for a host that should not run background
@@ -136,21 +138,29 @@ await users.GetSecurityStampAsync(userId, context.HttpContext.RequestAborted) is
     app.MapScalarApiReference();   // interactive API UI at /scalar/v1
 }
 
+// First, so it wraps every other piece of middleware: anything that throws below this point
+// becomes a correlated 500 rather than an empty one.
+app.UseWidgetWorksExceptionHandler();
+
 app.UseCors(SpaCorsPolicy);
 
+// Watches real traffic for the proxy misconfiguration that would otherwise turn per-caller
+// throttling into a global cap without anything saying so.
+app.Use(async (context, next) =>
+{
+    context.RequestServices.GetRequiredService<ProxyConfigurationCheck>().Inspect(context);
+    await next(context);
+});
+
 // Ahead of authentication on purpose: a throttled request is rejected before the app spends
 // work validating credentials, which is what keeps a guessing flood cheap to absorb.
 app.UseRateLimiter();
 app.UseAuthentication();
 app.UseAuthorization();
 
-// 200 only when the database is actually usable. A 503 naming the failure is what turns a silent
-// restart loop into a one-line diagnosis.
-app.MapGet("/health", (TimeProvider clock) => migration.Successful
-    ? Results.Ok(new { status = "ok", utcNow = clock.GetUtcNow() })
-    : Results.Json(
-        new { status = "unhealthy", reason = "database migration failed", detail = migration.Error, utcNow = clock.GetUtcNow() },
-        statusCode: StatusCodes.Status503ServiceUnavailable));
+// Liveness at /health (cheap, no database — the keep-warm schedule pings it) and readiness at
+// /health/ready (queries the database, for platform probes and alerting).
+app.MapHealthEndpoints(migration.Successful, migration.Error);
 app.MapAuthEndpoints();
 app.MapSecurityEndpoints();
 app.MapTwoFactorEndpoints();

src/WidgetWorks.WebApi/RateLimiting/ProxyConfigurationCheck.cs

@@ -0,0 +1,57 @@
+namespace WidgetWorks.WebApi.RateLimiting;
+
+/// <summary>
+/// Notices the one configuration mistake that turns throttling into an outage, and says so.
+///
+/// Rate limiting partitions by caller. Behind a reverse proxy every request arrives carrying the
+/// proxy's address, so unless <c>X-Forwarded-For</c> is trusted, every caller in the world collapses
+/// into a single partition and the limiter becomes a global cap that the first busy minute trips for
+/// everybody. Nothing about that is visible in a log: requests simply start returning 429.
+///
+/// The inverse mistake is the security one — trusting the header with no proxy in front lets a
+/// caller forge it and mint a fresh partition per request, opting out of throttling entirely.
+///
+/// Both are silent, so this watches real traffic and warns once for whichever it sees.
+/// </summary>
+public sealed class ProxyConfigurationCheck(RateLimitOptions options, ILogger<ProxyConfigurationCheck> logger)
+{
+    private int _warned;
+
+    /// <summary>Inspects one request. Cheap after the first warning, and warns at most once.</summary>
+    public void Inspect(HttpContext context)
+    {
+        ArgumentNullException.ThrowIfNull(context);
+
+        if (Volatile.Read(ref _warned) != 0)
+        {
+            return;
+        }
+
+        var forwarded = context.Request.Headers.ContainsKey("X-Forwarded-For");
+
+        if (forwarded && !options.TrustForwardedFor)
+        {
+            WarnOnce(
+                "Requests carry X-Forwarded-For but RateLimiting:TrustForwardedFor is false, so every " +
+                "caller shares one throttling partition and the limits apply to all traffic combined. " +
+                "Set it true if a trusted proxy sits in front of this app.");
+        }
+        else if (!forwarded && options.TrustForwardedFor)
+        {
+            WarnOnce(
+                "RateLimiting:TrustForwardedFor is true but requests arrive without X-Forwarded-For. " +
+                "If no proxy is in front, a caller can forge that header and give itself an " +
+                "unlimited number of throttling partitions. Set it false unless a proxy is guaranteed.");
+        }
+    }
+
+    private void WarnOnce(string message)
+    {
+        // Interlocked so a burst of concurrent requests produces one warning rather than a page of
+        // identical ones, which is how a real signal gets scrolled past.
+        if (Interlocked.Exchange(ref _warned, 1) == 0)
+        {
+            logger.LogWarning("{Message}", message);
+        }
+    }
+}