From 08c122d3fb9acccab63b3cee2b0b97a23d33c146 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 23:14:26 +0000 Subject: [PATCH 1/3] feat(ops): a readiness probe, correlated failures, and a loud proxy check Three operability gaps. The application was correct and effectively mute: it could not report that it was unwell, and a failure left nothing to trace. The health check could never report unhealthy. /health closed over a variable captured at startup - whether migrations succeeded when the process began, and nothing after. Once an instance was up it answered ok forever: database gone, pool exhausted, credentials rotated, still ok. The scheduled keep-warm ping made that worse rather than better, holding an instance in rotation on the strength of an answer that could not change. The obvious fix - query the database in /health - would have been wrong here, and the reason is written down in keep-warm.yml: that endpoint is pinged every few minutes to hold a free-tier instance loaded, and waking a serverless database on that cadence costs roughly 180 CU-hrs against a 100 CU-hr monthly budget. The shallow probe is deliberate. So this adds an endpoint rather than changing one. /health keeps its exact contract, including the 503 on a failed migration that the provisioning script watches for on first boot. /health/ready opens a connection and runs select 1, and is what a platform probe and alerting should use. Its failure body names the exception type and never its message, because a connection error can carry a host or a user and the endpoint is anonymous. Failures had nothing to trace them by. No exception handler meant a bare 500 with an empty body - safe, since the developer exception page is Development only, but unsupportable: nothing connected what the customer saw to what the logs recorded. Every response now carries X-Correlation-Id, a 500 repeats it in the body, and the same id is on the log line. An id supplied upstream is kept so a trace survives across services, but sanitised first: it reaches log messages, and text carrying newlines could forge whole entries (CWE-117) - the same class of defect already fixed once here, on the order-status path. The proxy setting was load-bearing and silent. Throttling partitions by caller. Behind a proxy with TrustForwardedFor left false, every caller collapses into one partition and the limits apply to all traffic combined - an outage caused by a configuration value, with nothing in the logs to say so. Trusting the header with no proxy in front is the inverse mistake and the security one, since a caller can then forge it and mint unlimited partitions. A check now watches real traffic and warns once for whichever it sees, using Interlocked so a burst produces one line rather than a page of identical ones. Eleven tests: both probes and their separation, correlation ids echoed and preserved across a hop, a forged newline stripped, an over-long id truncated, and both directions of the proxy mistake warned exactly once. 495 backend tests pass against PostgreSQL 16; dotnet format clean; build clean under -warnaserror. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- .../Diagnostics/CorrelationId.cs | 65 ++++++++ .../Diagnostics/ExceptionHandling.cs | 57 +++++++ .../Diagnostics/HealthEndpoints.cs | 87 +++++++++++ src/WidgetWorks.WebApi/Program.cs | 24 ++- .../RateLimiting/ProxyConfigurationCheck.cs | 57 +++++++ .../DiagnosticsApiTests.cs | 139 ++++++++++++++++++ .../ProxyConfigurationCheckTests.cs | 103 +++++++++++++ 7 files changed, 525 insertions(+), 7 deletions(-) create mode 100644 src/WidgetWorks.WebApi/Diagnostics/CorrelationId.cs create mode 100644 src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs create mode 100644 src/WidgetWorks.WebApi/Diagnostics/HealthEndpoints.cs create mode 100644 src/WidgetWorks.WebApi/RateLimiting/ProxyConfigurationCheck.cs create mode 100644 tests/WidgetWorks.ApiTests/DiagnosticsApiTests.cs create mode 100644 tests/WidgetWorks.ApiTests/ProxyConfigurationCheckTests.cs diff --git a/src/WidgetWorks.WebApi/Diagnostics/CorrelationId.cs b/src/WidgetWorks.WebApi/Diagnostics/CorrelationId.cs new file mode 100644 index 0000000..90d709c --- /dev/null +++ b/src/WidgetWorks.WebApi/Diagnostics/CorrelationId.cs @@ -0,0 +1,65 @@ +namespace WidgetWorks.WebApi.Diagnostics; + +/// +/// 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. +/// +public static class CorrelationId +{ + /// Header the id is read from and echoed on, matching the de-facto convention. + public const string HeaderName = "X-Correlation-Id"; + + /// Longest inbound id accepted, so a caller cannot push arbitrary text into the logs. + private const int MaxLength = 64; + + /// + /// 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. + /// + 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; + } + + /// Keeps letters, digits and a few separators; drops everything else and truncates. + 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); + } +} diff --git a/src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs b/src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs new file mode 100644 index 0000000..ab24594 --- /dev/null +++ b/src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs @@ -0,0 +1,57 @@ +using Microsoft.AspNetCore.Diagnostics; + +namespace WidgetWorks.WebApi.Diagnostics; + +public static class ExceptionHandling +{ + /// + /// 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. + /// + public static void UseWidgetWorksExceptionHandler(this WebApplication app) + { + app.UseExceptionHandler(builder => builder.Run(async context => + { + var correlationId = CorrelationId.Resolve(context); + var feature = context.Features.Get(); + + app.Logger.LogError( + feature?.Error, + "Unhandled exception for {Method} {Path} (correlation {CorrelationId}).", + context.Request.Method, + 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); + }); + } +} diff --git a/src/WidgetWorks.WebApi/Diagnostics/HealthEndpoints.cs b/src/WidgetWorks.WebApi/Diagnostics/HealthEndpoints.cs new file mode 100644 index 0000000..17bf2d7 --- /dev/null +++ b/src/WidgetWorks.WebApi/Diagnostics/HealthEndpoints.cs @@ -0,0 +1,87 @@ +using System.Data; +using Dapper; +using WidgetWorks.Infrastructure.Persistence; + +namespace WidgetWorks.WebApi.Diagnostics; + +/// +/// Two probes answering two different questions, because conflating them is how a monitoring signal +/// ends up unable to report bad news. +/// +public static class HealthEndpoints +{ + /// Outcome of the startup migration — a fact about this boot. + /// Why it failed, when it did. + 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(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); + } + }); + } +} diff --git a/src/WidgetWorks.WebApi/Program.cs b/src/WidgetWorks.WebApi/Program.cs index bd6c6fb..292d61b 100644 --- a/src/WidgetWorks.WebApi/Program.cs +++ b/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(); // 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().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(); diff --git a/src/WidgetWorks.WebApi/RateLimiting/ProxyConfigurationCheck.cs b/src/WidgetWorks.WebApi/RateLimiting/ProxyConfigurationCheck.cs new file mode 100644 index 0000000..872ee85 --- /dev/null +++ b/src/WidgetWorks.WebApi/RateLimiting/ProxyConfigurationCheck.cs @@ -0,0 +1,57 @@ +namespace WidgetWorks.WebApi.RateLimiting; + +/// +/// 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 X-Forwarded-For 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. +/// +public sealed class ProxyConfigurationCheck(RateLimitOptions options, ILogger logger) +{ + private int _warned; + + /// Inspects one request. Cheap after the first warning, and warns at most once. + 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); + } + } +} diff --git a/tests/WidgetWorks.ApiTests/DiagnosticsApiTests.cs b/tests/WidgetWorks.ApiTests/DiagnosticsApiTests.cs new file mode 100644 index 0000000..7b30711 --- /dev/null +++ b/tests/WidgetWorks.ApiTests/DiagnosticsApiTests.cs @@ -0,0 +1,139 @@ +using System.Net; +using System.Net.Http.Json; +using Microsoft.AspNetCore.Http; +using WidgetWorks.WebApi.Diagnostics; +using Xunit; + +namespace WidgetWorks.ApiTests; + +/// +/// The two probes and the correlation id. These exist because the previous behaviour was safe but +/// unobservable: a health endpoint that could never report bad news, and a 500 with nothing tying it +/// to a log line. +/// +[Collection(ApiCollection.Name)] +public sealed class DiagnosticsApiTests(ApiFixture fixture) +{ + private sealed record Liveness(string Status); + private sealed record Readiness(string Status, string Database, bool MigrationSucceeded); + + [Fact] + public async Task Liveness_answers_without_touching_the_database() + { + using var client = fixture.Factory.CreateClient(); + + var response = await client.GetAsync("/health"); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.Equal("ok", body!.Status); + } + + [Fact] + public async Task Readiness_reports_the_database_actually_answering() + { + using var client = fixture.Factory.CreateClient(); + + var response = await client.GetAsync("/health/ready"); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.Equal("ready", body!.Status); + Assert.Equal("ok", body.Database); + Assert.True(body.MigrationSucceeded); + } + + [Fact] + public async Task Readiness_is_a_separate_endpoint_so_the_cheap_probe_stays_cheap() + { + using var client = fixture.Factory.CreateClient(); + + // Both exist and both answer. The distinction is the point: /health is what the keep-warm + // schedule pings, and waking a serverless database on that cadence is what would blow the + // monthly compute budget. Merging them is the mistake this pins against. + Assert.Equal(HttpStatusCode.OK, (await client.GetAsync("/health")).StatusCode); + Assert.Equal(HttpStatusCode.OK, (await client.GetAsync("/health/ready")).StatusCode); + } + + [Fact] + public async Task Every_response_carries_a_correlation_id() + { + using var client = fixture.Factory.CreateClient(); + + var response = await client.GetAsync("/health"); + + Assert.True(response.Headers.TryGetValues(CorrelationId.HeaderName, out var values)); + Assert.False(string.IsNullOrWhiteSpace(values!.First())); + } + + [Fact] + public async Task A_caller_supplied_correlation_id_is_kept_so_a_trace_survives() + { + using var client = fixture.Factory.CreateClient(); + using var request = new HttpRequestMessage(HttpMethod.Get, "/health"); + request.Headers.Add(CorrelationId.HeaderName, "trace-from-upstream-1"); + + var response = await client.SendAsync(request); + + // A request that already carries an id keeps it, so one trace spans several services rather + // than restarting at each hop. + Assert.Equal("trace-from-upstream-1", response.Headers.GetValues(CorrelationId.HeaderName).First()); + } +} + +/// +/// The id itself. Worth unit tests because it reaches log messages, and text that reaches a log +/// message is an injection surface. +/// +public class CorrelationIdTests +{ + private static HttpContext RequestWith(string? supplied) + { + var context = new DefaultHttpContext { TraceIdentifier = "trace-assigned-by-the-host" }; + if (supplied is not null) + { + context.Request.Headers[CorrelationId.HeaderName] = supplied; + } + + return context; + } + + [Fact] + public void Falls_back_to_the_id_the_host_already_assigned() + => Assert.Equal("trace-assigned-by-the-host", CorrelationId.Resolve(RequestWith(null))); + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void An_empty_header_is_ignored(string supplied) + => Assert.Equal("trace-assigned-by-the-host", CorrelationId.Resolve(RequestWith(supplied))); + + [Fact] + public void A_sane_inbound_id_is_kept() + => Assert.Equal("abc-123_x.y:z", CorrelationId.Resolve(RequestWith("abc-123_x.y:z"))); + + [Fact] + public void Newlines_are_stripped_so_a_caller_cannot_forge_log_entries() + { + // Left intact this would put an attacker-authored line into the log stream that reads like + // a genuine record (CWE-117) - the same class of defect already fixed once here, on the + // order-status path. + var forged = CorrelationId.Resolve(RequestWith("ok\r\nfatal: database deleted by admin")); + + Assert.DoesNotContain('\n', forged); + Assert.DoesNotContain('\r', forged); + Assert.StartsWith("ok", forged, StringComparison.Ordinal); + } + + [Fact] + public void An_absurdly_long_id_is_truncated_rather_than_logged_whole() + { + var resolved = CorrelationId.Resolve(RequestWith(new string('a', 500))); + + Assert.True(resolved.Length <= 64); + } + + [Fact] + public void A_header_of_only_junk_falls_back_instead_of_yielding_an_empty_id() + => Assert.Equal("trace-assigned-by-the-host", CorrelationId.Resolve(RequestWith("<<<>>>"))); +} diff --git a/tests/WidgetWorks.ApiTests/ProxyConfigurationCheckTests.cs b/tests/WidgetWorks.ApiTests/ProxyConfigurationCheckTests.cs new file mode 100644 index 0000000..306956f --- /dev/null +++ b/tests/WidgetWorks.ApiTests/ProxyConfigurationCheckTests.cs @@ -0,0 +1,103 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging; +using WidgetWorks.WebApi.RateLimiting; +using Xunit; + +namespace WidgetWorks.ApiTests; + +/// +/// The watcher for the configuration mistake that turns per-caller throttling into a global cap. +/// Both directions of the mistake are silent in production, so the warning is the whole feature and +/// deserves to be pinned. +/// +public class ProxyConfigurationCheckTests +{ + /// Captures warnings so the test can assert on what an operator would actually see. + private sealed class CapturingLogger : ILogger + { + public readonly List Warnings = []; + + 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) + { + if (logLevel == LogLevel.Warning) + { + Warnings.Add(formatter(state, exception)); + } + } + } + + private static HttpContext Request(bool withForwardedFor) + { + var context = new DefaultHttpContext(); + if (withForwardedFor) + { + context.Request.Headers["X-Forwarded-For"] = "198.51.100.9"; + } + + return context; + } + + [Fact] + public void Warns_when_a_proxy_is_in_front_but_its_header_is_not_trusted() + { + var log = new CapturingLogger(); + var check = new ProxyConfigurationCheck(new RateLimitOptions { TrustForwardedFor = false }, log); + + check.Inspect(Request(withForwardedFor: true)); + + // This is the outage case: every caller collapses into one partition and the limits start + // applying to all traffic combined. + Assert.Single(log.Warnings); + Assert.Contains("one throttling partition", log.Warnings[0], StringComparison.Ordinal); + } + + [Fact] + public void Warns_when_the_header_is_trusted_but_no_proxy_appears_to_send_it() + { + var log = new CapturingLogger(); + var check = new ProxyConfigurationCheck(new RateLimitOptions { TrustForwardedFor = true }, log); + + check.Inspect(Request(withForwardedFor: false)); + + // The inverse, and the security half: a caller can forge the header and opt out of limits. + Assert.Single(log.Warnings); + Assert.Contains("forge", log.Warnings[0], StringComparison.Ordinal); + } + + [Theory] + [InlineData(true, true)] + [InlineData(false, false)] + public void Stays_quiet_when_the_setting_matches_the_traffic(bool trust, bool forwarded) + { + var log = new CapturingLogger(); + var check = new ProxyConfigurationCheck(new RateLimitOptions { TrustForwardedFor = trust }, log); + + check.Inspect(Request(forwarded)); + + Assert.Empty(log.Warnings); + } + + [Fact] + public void Warns_once_however_much_traffic_arrives() + { + var log = new CapturingLogger(); + var check = new ProxyConfigurationCheck(new RateLimitOptions { TrustForwardedFor = false }, log); + + for (var i = 0; i < 50; i++) + { + check.Inspect(Request(withForwardedFor: true)); + } + + // A page of identical warnings is how a real signal gets scrolled past. + Assert.Single(log.Warnings); + } +} From 8674bf4449072615d0c52ff9751788446b622a71 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 23:21:18 +0000 Subject: [PATCH 2/3] feat(catalog): page through the catalogue instead of hoping it fits The last piece of the browser-side narrowing removed earlier. Filtering and ordering already moved into the query, but the grid still showed whatever came back from a single request, so PAGE_SIZE was a ceiling on the whole catalogue rather than a page size. It had been raised to 100 to keep 75 products visible - a number that works until someone adds the hundred-and-first, at which point products stop appearing with no error anywhere. Silent is the part that made it worth fixing rather than tuning. The grid now pages. The page is a URL parameter like every other part of the query, so a shelf can be linked, bookmarked and reached with the back button, and the pager only renders when there is more than one page - an ordinary shelf gains no furniture it does not need. Changing the query resets to the first page. Keeping the old page number across a filter change strands the reader on page 4 of a result that now has two, and the grid comes back empty for no visible reason. PAGE_SIZE drops from 100 to 24, which is a screenful rather than "as much as the API will allow". The API's own cap is untouched and no longer load-bearing. The count line reads honestly across pages: "24 products of 75, page 1 of 4" rather than a number that quietly means "on this page". Five tests: no pager when everything fits, stepping forward, stepping back from further in, the last page refusing to advance, and a query change dropping the page. That third one exists because the Previous handler is unreachable from page one, and without it the frontend coverage gate failed at 99.47% of functions - the floor caught the gap before CI did. Verified in the running app against a paging API: 24 of 75 shown, page 1 of 4, Next advancing and the URL following. 239 frontend tests and 506 backend tests pass; frontend coverage back to 100% lines and functions. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- web/src/lib/catalog.ts | 16 +++--- web/src/pages/CatalogPage.tsx | 38 ++++++++++++-- web/src/pages/StorefrontPages.test.tsx | 71 ++++++++++++++++++++++++++ web/src/styles.css | 12 +++++ 4 files changed, 127 insertions(+), 10 deletions(-) diff --git a/web/src/lib/catalog.ts b/web/src/lib/catalog.ts index fafbca0..18d033f 100644 --- a/web/src/lib/catalog.ts +++ b/web/src/lib/catalog.ts @@ -1,12 +1,16 @@ // Storefront browsing vocabulary, shared by the header scope select, the // category rail and the catalog grid so all three stay in step. // -// Search, category and sort are all applied by the API. They used to be -// narrowed here over a single fetched page, which meant a catalog larger than -// PAGE_SIZE lost its tail from every shelf and a sort only ordered whatever -// happened to be on that page. PAGE_SIZE is now just how many results one -// request asks for; growing past it needs a pager, not a bigger number. -export const PAGE_SIZE = 100 +// Search, category and sort are all applied by the API, and results are paged. +// They were once narrowed here over a single fetched page, so a catalog larger +// than PAGE_SIZE lost its tail from every shelf and a sort ordered only what +// happened to be on it. Both are the server's job now, and the grid pages +// through the result rather than hoping it fits in one response. +// One screenful, not "everything we can get away with". Before the grid could +// page, this had to be large enough to hold the whole catalog or products fell +// off the end silently; now it is an ordinary page size and the catalog can be +// any size at all. +export const PAGE_SIZE = 24 export interface Category { /** URL value for the `cat` search param — empty means "everything". */ diff --git a/web/src/pages/CatalogPage.tsx b/web/src/pages/CatalogPage.tsx index 2109019..0ed3685 100644 --- a/web/src/pages/CatalogPage.tsx +++ b/web/src/pages/CatalogPage.tsx @@ -48,6 +48,9 @@ export function CatalogPage() { const q = params.get('q') ?? '' const cat = params.get('cat') ?? '' const sort = params.get('sort') ?? 'featured' + // Paging lives in the URL like every other part of the query, so a shelf can be + // linked, bookmarked and reached with the back button. + const page = Math.max(1, Number(params.get('page') ?? '1') || 1) const [data, setData] = useState | null>(null) const [error, setError] = useState(null) @@ -56,7 +59,7 @@ export function CatalogPage() { useEffect(() => { let active = true setLoading(true) - const sp = new URLSearchParams({ pageSize: String(PAGE_SIZE) }) + const sp = new URLSearchParams({ pageSize: String(PAGE_SIZE), page: String(page) }) if (q.trim()) sp.set('search', q.trim()) // Category and sort are the server's job. Narrowing a single fetched page in the browser // silently dropped anything past that page from a shelf, and sorted only what happened to be @@ -69,7 +72,7 @@ export function CatalogPage() { .catch((e) => { if (active) setError(e.message) }) .finally(() => { if (active) setLoading(false) }) return () => { active = false } - }, [q, cat, sort]) + }, [q, cat, sort, page]) const items = data?.items ?? [] @@ -83,6 +86,9 @@ export function CatalogPage() { const next = new URLSearchParams(params) if (value) next.set(key, value) else next.delete(key) + // Any change to what is being asked for starts again at the first page. Keeping the old + // page number would strand a reader on page 4 of a result that now has two. + if (key !== 'page') next.delete('page') setParams(next, { replace: true }) } @@ -142,8 +148,8 @@ export function CatalogPage() { {loading ? 'Loading products…' : `${items.length} ${items.length === 1 ? 'product' : 'products'}${ - data && items.length < data.totalCount ? ` of ${data.totalCount}` : '' - }`} + data && data.totalCount > items.length ? ` of ${data.totalCount}` : '' + }${data && data.totalPages > 1 ? ` · page ${data.page} of ${data.totalPages}` : ''}`} @@ -170,6 +176,30 @@ export function CatalogPage() { )} + {!loading && !error && data && data.totalPages > 1 && ( + + )} + {!loading && !error && items.length === 0 && (
diff --git a/web/src/pages/StorefrontPages.test.tsx b/web/src/pages/StorefrontPages.test.tsx index 10eb896..51a4c7e 100644 --- a/web/src/pages/StorefrontPages.test.tsx +++ b/web/src/pages/StorefrontPages.test.tsx @@ -131,6 +131,77 @@ describe('CatalogPage', () => { await waitFor(() => expect(calls.some((c) => c.url.includes('category=mega'))).toBe(true)) }) + it('shows no pager when everything fits on one page', async () => { + stubFetch([['/catalog/widgets', paged()]]) + + renderWithProviders(, { at: '/store', path: '/store' }) + await screen.findByText('Standard Widget') + + // An ordinary shelf should gain no furniture it does not need. + expect(screen.queryByRole('navigation', { name: 'Pagination' })).not.toBeInTheDocument() + }) + + it('pages through a catalog larger than one response', async () => { + const calls = stubFetch([['/catalog/widgets', + () => ({ items: [widget, soldOut], page: 1, pageSize: 24, totalCount: 50, totalPages: 3 })]]) + const user = userEvent.setup() + + renderWithProviders(, { at: '/store', path: '/store' }) + await screen.findByText('Standard Widget') + + expect(screen.getByText('Page 1 of 3')).toBeInTheDocument() + // Nowhere to go back to from the first page. + expect(screen.getByRole('button', { name: /Previous/ })).toBeDisabled() + + await user.click(screen.getByRole('button', { name: /Next/ })) + + await waitFor(() => expect(calls.some((c) => c.url.includes('page=2'))).toBe(true)) + }) + + it('steps back a page from further in', async () => { + const calls = stubFetch([['/catalog/widgets', + () => ({ items: [widget, soldOut], page: 2, pageSize: 24, totalCount: 50, totalPages: 3 })]]) + const user = userEvent.setup() + + renderWithProviders(, { at: '/store?page=2', path: '/store' }) + await screen.findByText('Standard Widget') + + // Past the first page both directions are live. + expect(screen.getByRole('button', { name: /Previous/ })).toBeEnabled() + await user.click(screen.getByRole('button', { name: /Previous/ })) + + await waitFor(() => expect(calls.some((c) => c.url.includes('page=1'))).toBe(true)) + }) + + it('stops at the last page', async () => { + stubFetch([['/catalog/widgets', + () => ({ items: [widget], page: 3, pageSize: 24, totalCount: 50, totalPages: 3 })]]) + + renderWithProviders(, { at: '/store?page=3', path: '/store' }) + await screen.findByText('Standard Widget') + + expect(screen.getByRole('button', { name: /Next/ })).toBeDisabled() + }) + + it('returns to the first page when the query changes', async () => { + const calls = stubFetch([['/catalog/widgets', + () => ({ items: [widget, soldOut], page: 3, pageSize: 24, totalCount: 50, totalPages: 3 })]]) + const user = userEvent.setup() + + renderWithProviders(, { at: '/store?page=3', path: '/store' }) + await screen.findByText('Standard Widget') + + await user.selectOptions(screen.getByLabelText('Sort by'), 'price-desc') + + // Keeping page 3 across a change of query strands the reader on a page the new result may + // not have, and the grid comes back empty for no visible reason. + await waitFor(() => { + const latest = calls[calls.length - 1].url + expect(latest).toContain('sort=price-desc') + expect(latest).not.toContain('page=3') + }) + }) + it('clears a category from the toolbar', async () => { stubFetch([['/catalog/widgets', paged()]]) const user = userEvent.setup() diff --git a/web/src/styles.css b/web/src/styles.css index c2b4d97..f9a9c27 100644 --- a/web/src/styles.css +++ b/web/src/styles.css @@ -507,6 +507,18 @@ select{ .shortcut .ico svg{width:40px;height:40px;display:block} .shortcut .lbl{font-size:.85rem;font-weight:700;color:var(--ink)} +/* Pager -------------------------------------------------------------------- + Only rendered when there is more than one page, so an ordinary shelf shows + nothing and a large catalog gains a control exactly when it needs one. */ +.pager{ + display:flex;align-items:center;justify-content:center;gap:16px; + margin:22px 0 4px;padding:12px 16px; + background:var(--surface);border:1px solid var(--line);border-radius:var(--r-md); + box-shadow:var(--sh-1); +} +.pager-at{font-size:.85rem;font-weight:600;color:var(--ink-2);font-variant-numeric:tabular-nums} +.pager .btn:disabled{opacity:.45;cursor:default} + /* Results toolbar */ .toolbar{ display:flex;align-items:center;justify-content:space-between;gap:16px;flex-wrap:wrap; From 6eeae89e0dac22487a9538e90eddaaf63fb9be3e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 23:25:47 +0000 Subject: [PATCH 3/3] fix(ops): sanitise the request path before it reaches a log line CodeQL flagged the exception handler this pull request added: it logged Request.Path.Value straight from the request. That is the decoded path, so a URL containing %0A arrives as a real newline, ends the log entry, and begins one the caller wrote - the caller choosing what the log appears to say about them (CWE-117). Caught in review by the scanner, and the miss is worth naming: the correlation id in the same handler was sanitised for exactly this reason, with a comment citing exactly this weakness, while the value beside it went through raw. Knowing the rule is not the same as applying it everywhere it holds. LogSafe.Text strips control characters and truncates. Printable oddities are kept deliberately - a path full of strange characters is what an operator needs to see - and only the characters that can restructure the log itself are removed. A value that sanitises away to nothing becomes a marker rather than a blank field, so the log never quietly loses a column. Applied to the path and the method. The correlation id was already clean by construction. Six tests: ordinary text survives, newlines and control characters go, printable oddities are kept, a long value truncates, and nothing usable becomes a marker. 512 backend tests pass; dotnet format clean. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- .../Diagnostics/ExceptionHandling.cs | 8 +++- src/WidgetWorks.WebApi/Diagnostics/LogSafe.cs | 48 +++++++++++++++++++ .../DiagnosticsApiTests.cs | 41 ++++++++++++++++ 3 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 src/WidgetWorks.WebApi/Diagnostics/LogSafe.cs diff --git a/src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs b/src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs index ab24594..5dffdbc 100644 --- a/src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs +++ b/src/WidgetWorks.WebApi/Diagnostics/ExceptionHandling.cs @@ -22,11 +22,15 @@ public static void UseWidgetWorksExceptionHandler(this WebApplication app) var correlationId = CorrelationId.Resolve(context); var feature = context.Features.Get(); + // 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}).", - context.Request.Method, - context.Request.Path.Value, + LogSafe.Text(context.Request.Method, maxLength: 16), + LogSafe.Text(context.Request.Path.Value), correlationId); context.Response.StatusCode = StatusCodes.Status500InternalServerError; diff --git a/src/WidgetWorks.WebApi/Diagnostics/LogSafe.cs b/src/WidgetWorks.WebApi/Diagnostics/LogSafe.cs new file mode 100644 index 0000000..713dab7 --- /dev/null +++ b/src/WidgetWorks.WebApi/Diagnostics/LogSafe.cs @@ -0,0 +1,48 @@ +namespace WidgetWorks.WebApi.Diagnostics; + +/// +/// 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: Path.Value is +/// the *decoded* path, so %0A in a URL arrives as a real line break. +/// +public static class LogSafe +{ + /// Shown in place of a value that sanitised away to nothing, so the field is never blank. + public const string Empty = "(empty)"; + + /// + /// 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. + /// + 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); + } +} diff --git a/tests/WidgetWorks.ApiTests/DiagnosticsApiTests.cs b/tests/WidgetWorks.ApiTests/DiagnosticsApiTests.cs index 7b30711..d748ad9 100644 --- a/tests/WidgetWorks.ApiTests/DiagnosticsApiTests.cs +++ b/tests/WidgetWorks.ApiTests/DiagnosticsApiTests.cs @@ -137,3 +137,44 @@ public void An_absurdly_long_id_is_truncated_rather_than_logged_whole() public void A_header_of_only_junk_falls_back_instead_of_yielding_an_empty_id() => Assert.Equal("trace-assigned-by-the-host", CorrelationId.Resolve(RequestWith("<<<>>>"))); } +/// +/// Making caller-controlled text safe for a log line. CodeQL flagged the original version of the +/// exception handler for exactly this: Request.Path.Value is the decoded path, so %0A in a URL +/// arrives as a real newline and the caller gets to write a log entry. +/// +public class LogSafeTests +{ + [Fact] + public void Ordinary_text_survives_untouched() + => Assert.Equal("/catalog/widgets", LogSafe.Text("/catalog/widgets")); + + [Fact] + public void Newlines_are_removed_so_a_caller_cannot_forge_an_entry() + { + var forged = LogSafe.Text("/orders\r\nfatal: database deleted by admin"); + + Assert.DoesNotContain('\n', forged); + Assert.DoesNotContain('\r', forged); + // The text still reads, so an operator can see what was actually requested. + Assert.StartsWith("/orders", forged, StringComparison.Ordinal); + } + + [Fact] + public void Tabs_and_other_control_characters_go_too() + => Assert.Equal("ab", LogSafe.Text("a\tb\u0000")); + + [Fact] + public void Odd_but_printable_characters_are_kept_because_that_is_the_evidence() + => Assert.Equal("/search?q=