diff --git a/deploy/.env.example b/deploy/.env.example index 60a74bae..1981c205 100644 --- a/deploy/.env.example +++ b/deploy/.env.example @@ -38,6 +38,12 @@ Seed__AdminPassword=replace-with-a-strong-password # RateLimiting__Refresh__PermitLimit=60 # RateLimiting__Refresh__WindowSeconds=900 +# Browser error reports (#217): the anonymous /api/v1/client-errors endpoint is +# throttled per client IP so it can't be used to flood the log. The default is +# enough for a genuinely crashing screen, tiny for an abuser. +# RateLimiting__ClientErrors__PermitLimit=10 +# RateLimiting__ClientErrors__WindowSeconds=300 + # OpenTelemetry export (#214 traces, #215 metrics). Both signals go over OTLP # to the SAME endpoint, ONLY when it is set; leave it unset to keep telemetry # in-process (no exporter, no overhead). A malformed endpoint or protocol diff --git a/src/Cluckwork.Api/Dockerfile b/src/Cluckwork.Api/Dockerfile index aeaa6bf7..660af343 100644 --- a/src/Cluckwork.Api/Dockerfile +++ b/src/Cluckwork.Api/Dockerfile @@ -5,6 +5,13 @@ COPY web/package.json web/package-lock.json ./ RUN npm ci COPY web/ ./ RUN npm run build +# #217 — sourcemaps are an OPERATOR tool, not a public asset. The build emits +# them "hidden" (no sourceMappingURL comment), but anything left in dist/ ends +# up in wwwroot where a predictable ".js.map" URL would serve the full +# original sources to anyone. Park them next to the app instead: resolve a +# reported stack with docker cp :/app/sourcemaps . +RUN mkdir /web/sourcemaps && \ + find /web/dist -name '*.map' -exec mv {} /web/sourcemaps/ \; # --- API build: restore + publish the .NET app --- FROM mcr.microsoft.com/dotnet/sdk:10.0 AS build @@ -26,4 +33,5 @@ EXPOSE 8080 FROM runtime AS final COPY --from=build /out . COPY --from=web-build /web/dist ./wwwroot +COPY --from=web-build /web/sourcemaps ./sourcemaps ENTRYPOINT ["dotnet", "Cluckwork.Api.dll"] diff --git a/src/Cluckwork.Api/Endpoints/ClientErrors/ClientErrorEndpoints.cs b/src/Cluckwork.Api/Endpoints/ClientErrors/ClientErrorEndpoints.cs new file mode 100644 index 00000000..80e98e24 --- /dev/null +++ b/src/Cluckwork.Api/Endpoints/ClientErrors/ClientErrorEndpoints.cs @@ -0,0 +1,153 @@ +namespace Cluckwork.Api.Endpoints.ClientErrors; + +using System.Text.Json; +using Cluckwork.Api.RateLimiting; +using Microsoft.AspNetCore.Http.Features; + +// #217 — the SPA's ErrorBoundary reports render crashes here so the operator +// learns a screen is crashing without a support screenshot. The report is +// WRITTEN TO THE LOG and stored nowhere else: no table, no retention question, +// and the existing log pipeline (console, OTLP when enabled) carries it. +// +// Everything about the endpoint assumes a hostile caller, because it is +// anonymous by design — the login screen can crash too, and a crashing +// authenticated app may no longer hold a usable token. Per-IP rate limit +// (#143 infrastructure), a total byte cap read under our control, and +// per-field truncation bound what one address can push into the log. +public static class ClientErrorEndpoints +{ + // Total request-body cap. A real report is a few KB of stack; 16 KB gives + // slack for deep component trees without letting one POST carry a novel. + public const int MaxReportBytes = 16 * 1024; + + // Per-field bounds applied AFTER the byte cap: the cap limits the request, + // these keep any single log line readable and bounded even at the cap. + private const int MaxMessageChars = 2000; + private const int MaxStackChars = 8000; + private const int MaxRouteChars = 500; + private const int MaxShortChars = 100; + + public static RouteGroupBuilder MapClientErrorEndpoints(this RouteGroupBuilder group) + { + group.MapPost("/", Report) + .AllowAnonymous() + .RequireRateLimiting(RateLimitingOptions.ClientErrorsPolicyName) + .WithName("ReportClientError") + .WithSummary("Accept a browser error report and write it to the server log."); + return group; + } + + // Bound manually (HttpRequest, not a [FromBody] DTO): model binding would + // buffer an arbitrarily large body before our code ran; here the read loop + // below is the size guarantee, same pattern as the logo upload (#123). + private static async Task Report( + HttpRequest request, ILogger logger, CancellationToken ct) + { + // A declared oversize is refused without reading a byte. Content-Length + // is only a claim, which is why the read below is capped as well. + if (request.ContentLength > MaxReportBytes) + return TooLarge(); + + // Best-effort transport cutoff (absent under TestServer, read-only once + // the body is touched); the loop bound is the guarantee, not this. + var sizeLimit = request.HttpContext.Features.Get(); + if (sizeLimit is { IsReadOnly: false }) + sizeLimit.MaxRequestBodySize = MaxReportBytes; + + // One byte of headroom: filling past MaxReportBytes proves the body is + // over the cap without reading whatever else the client meant to send. + var buffer = new byte[MaxReportBytes + 1]; + var read = 0; + while (read < buffer.Length) + { + var n = await request.Body.ReadAsync(buffer.AsMemory(read, buffer.Length - read), ct); + if (n == 0) break; + read += n; + } + if (read > MaxReportBytes) + return TooLarge(); + + ClientErrorReport? report; + try + { + report = JsonSerializer.Deserialize( + buffer.AsSpan(0, read), JsonOptions); + } + catch (JsonException) + { + return Results.Problem( + title: "Malformed report", statusCode: StatusCodes.Status400BadRequest, + detail: "The error report is not valid JSON."); + } + + if (report is null || string.IsNullOrWhiteSpace(report.Message)) + return Results.ValidationProblem(new Dictionary + { + ["message"] = ["A non-empty message is required."] + }); + + var scope = report.Scope?.ToLowerInvariant(); + if (scope is not ("app" or "screen")) + return Results.ValidationProblem(new Dictionary + { + ["scope"] = ["Scope must be \"app\" or \"screen\"."] + }); + + // Template carries the operator-facing line; the bulky fields ride as + // structured properties via the provider's scope handling (#216), so + // one query key (Scope/Route/ClientTraceId) finds them without the + // stacks shouting in the console template. + using (logger.BeginScope(new Dictionary + { + ["Stack"] = Truncate(report.Stack, MaxStackChars), + ["ComponentStack"] = Truncate(report.ComponentStack, MaxStackChars), + ["AppVersion"] = Truncate(report.AppVersion, MaxShortChars), + ["ClientTraceId"] = Truncate(report.TraceId, MaxShortChars) + })) + { + // Scope is validated to two literals above; Message and Route are + // the only anonymous-caller strings the console template RENDERS, + // so they get control characters stripped — CR/LF (or an ANSI + // escape) would let one report forge extra plain-text log lines. + // The stacks keep theirs: they ride as structured properties the + // console line never shows, and a stack without newlines is + // useless. + logger.LogError("Client error ({Scope}) at {Route}: {Message}", + scope, + Sanitize(Truncate(report.Route, MaxRouteChars)) ?? "(unknown)", + Sanitize(Truncate(report.Message, MaxMessageChars))); + } + + return Results.Accepted(); + } + + private static IResult TooLarge() => Results.Problem( + title: "Report too large", statusCode: StatusCodes.Status413PayloadTooLarge, + detail: $"Error reports are capped at {MaxReportBytes} bytes."); + + private static string? Truncate(string? value, int max) => + value is null || value.Length <= max ? value : value[..max]; + + private static string? Sanitize(string? value) => + value is null || !value.Any(char.IsControl) + ? value + : string.Create(value.Length, value, static (chars, source) => + { + for (var i = 0; i < source.Length; i++) + chars[i] = char.IsControl(source[i]) ? ' ' : source[i]; + }); + + private static readonly JsonSerializerOptions JsonOptions = + new(JsonSerializerDefaults.Web); +} + +// The report shape the SPA sends. Everything but the message and scope is +// optional: a crash report with holes still beats silence. +public sealed record ClientErrorReport( + string? Message, + string? Stack, + string? ComponentStack, + string? Scope, + string? Route, + string? AppVersion, + string? TraceId); diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index a028cb7a..85371498 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -6,6 +6,7 @@ using Cluckwork.Api.Endpoints.Audit; using Cluckwork.Api.Endpoints.Auth; using Cluckwork.Api.Endpoints.Catalog; +using Cluckwork.Api.Endpoints.ClientErrors; using Cluckwork.Api.Endpoints.Customers; using Cluckwork.Api.Endpoints.DailyEntries; using Cluckwork.Api.Endpoints.EggGrades; @@ -280,12 +281,13 @@ ((int)Math.Ceiling(retryAfter.TotalSeconds)).ToString(CultureInfo.InvariantCulture); await Results.Problem( title: "Too many requests", - detail: "Too many authentication attempts from this address. Try again later.", + detail: "Too many requests from this address. Try again later.", statusCode: StatusCodes.Status429TooManyRequests) .ExecuteAsync(context.HttpContext); }; AddFixedWindowByClientIp(limiter, RateLimitingOptions.LoginPolicyName, rateLimiting.Login); AddFixedWindowByClientIp(limiter, RateLimitingOptions.RefreshPolicyName, rateLimiting.Refresh); + AddFixedWindowByClientIp(limiter, RateLimitingOptions.ClientErrorsPolicyName, rateLimiting.ClientErrors); static void AddFixedWindowByClientIp( Microsoft.AspNetCore.RateLimiting.RateLimiterOptions limiter, @@ -721,6 +723,12 @@ static void AddFixedWindowByClientIp( .RequireAuthorization(AuthPolicies.AdminOnly) .MapExportEndpoints(); +// #217 — browser error reports. Anonymous (the login screen can crash too); +// the endpoint carries its own per-IP rate limit and size cap inside. +app.MapGroup("/api/v1/client-errors") + .WithTags("ClientErrors") + .MapClientErrorEndpoints(); + // Health: live = the process runs (no checks); ready = dependencies too. app.MapHealthChecks("/health/live", new Microsoft.AspNetCore.Diagnostics.HealthChecks.HealthCheckOptions { diff --git a/src/Cluckwork.Api/RateLimiting/RateLimitingOptions.cs b/src/Cluckwork.Api/RateLimiting/RateLimitingOptions.cs index 4f1c7bde..cdfb04d6 100644 --- a/src/Cluckwork.Api/RateLimiting/RateLimitingOptions.cs +++ b/src/Cluckwork.Api/RateLimiting/RateLimitingOptions.cs @@ -13,9 +13,14 @@ public sealed class RateLimitingOptions // behind one NAT IP starve the shared budget with normal refreshes (#143). public const string LoginPolicyName = "auth-login"; public const string RefreshPolicyName = "auth-refresh"; + // #217: the anonymous browser error-report endpoint. The budget guards the + // LOG, not a credential — enough for a genuinely crashing screen to get its + // story out, too little to flood the log from one address. + public const string ClientErrorsPolicyName = "client-errors"; public FixedWindow Login { get; init; } = new() { PermitLimit = 10, WindowSeconds = 900 }; public FixedWindow Refresh { get; init; } = new() { PermitLimit = 60, WindowSeconds = 900 }; + public FixedWindow ClientErrors { get; init; } = new() { PermitLimit = 10, WindowSeconds = 300 }; // Reverse-proxy networks whose X-Forwarded-For is honored (CIDR; /32 for a // single address). Fed to the framework ForwardedHeaders middleware, which @@ -37,6 +42,7 @@ public void Validate() { ValidateWindow(nameof(Login), Login); ValidateWindow(nameof(Refresh), Refresh); + ValidateWindow(nameof(ClientErrors), ClientErrors); ParseTrustedProxies(); // throws FormatException on a bad CIDR } diff --git a/tests/Cluckwork.Api.IntegrationTests/ClientErrorReportTests.cs b/tests/Cluckwork.Api.IntegrationTests/ClientErrorReportTests.cs new file mode 100644 index 00000000..7f8db75b --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/ClientErrorReportTests.cs @@ -0,0 +1,303 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Net; +using Cluckwork.Api.IntegrationTests.Infrastructure; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Serilog.Events; + +// #217 — the SPA's ErrorBoundary reports render crashes to the API, which +// writes them to the server log at Error level with structured fields. The +// endpoint is anonymous (the login screen can crash too), size-capped and +// rate-limited per IP so it cannot be used as a log-flooding vector, and +// stores nothing. +public sealed class ClientErrorReportFactory : CluckworkWebApplicationFactory +{ + public const int Limit = 3; + + public RequestLoggingFactory.CollectingSink Sink { get; } = new(); + + protected override void ConfigureWebHost(IWebHostBuilder builder) + { + base.ConfigureWebHost(builder); + builder.UseSetting("RateLimiting:ClientErrors:PermitLimit", Limit.ToString()); + builder.UseSetting("RateLimiting:ClientErrors:WindowSeconds", "900"); + builder.ConfigureTestServices(services => + { + // Program.cs pulls DI-registered sinks into the logger via + // ReadFrom.Services; this hands the test a live tap on every event. + services.AddSingleton(Sink); + services.AddSingleton(); + }); + } +} + +public sealed class ClientErrorReportTests(ClientErrorReportFactory factory) + : IClassFixture +{ + private const string Path = "/api/v1/client-errors"; + + // Distinct socket IP per test: the per-IP limiter must not let one test's + // reports eat another's budget. + private HttpClient ClientFrom(string ip) + { + var client = factory.CreateClient(); + client.DefaultRequestHeaders.Add("X-Test-Remote", ip); + return client; + } + + private IReadOnlyList ReportEvents() => + [.. factory.Sink.Events.Where(e => + ScalarOf(e, "SourceContext")?.Contains("ClientError") == true)]; + + private static string? ScalarOf(LogEvent e, string name) => + e.Properties.TryGetValue(name, out var value) && value is ScalarValue scalar + ? scalar.Value?.ToString() + : null; + + private static object ValidReport(string message = "boom") => new + { + message, + stack = "Error: boom\n at Crash (src/routes/DashboardPage.tsx:12:3)", + componentStack = "\n at Crash\n at ErrorBoundary", + scope = "screen", + route = "/daily-entries", + appVersion = "1.2.3", + traceId = "0123456789abcdef0123456789abcdef" + }; + + [Fact] + public async Task Valid_report_returns_202_and_logs_one_error_event_with_structured_fields() + { + var client = ClientFrom("203.0.113.101"); + var marker = $"crash-{Guid.NewGuid():N}"; + + var response = await client.PostAsJsonAsync(Path, ValidReport(marker)); + + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + var logged = Assert.Single(ReportEvents(), + e => ScalarOf(e, "Message")?.Contains(marker) == true); + Assert.Equal(LogEventLevel.Error, logged.Level); + Assert.Equal("screen", ScalarOf(logged, "Scope")); + Assert.Equal("/daily-entries", ScalarOf(logged, "Route")); + Assert.Contains("DashboardPage.tsx", ScalarOf(logged, "Stack")); + Assert.Contains("ErrorBoundary", ScalarOf(logged, "ComponentStack")); + Assert.Equal("1.2.3", ScalarOf(logged, "AppVersion")); + Assert.Equal("0123456789abcdef0123456789abcdef", ScalarOf(logged, "ClientTraceId")); + } + + [Fact] + public async Task App_scope_is_accepted_and_logged() + { + var client = ClientFrom("203.0.113.102"); + var marker = $"crash-{Guid.NewGuid():N}"; + + var response = await client.PostAsJsonAsync(Path, new + { + message = marker, + scope = "app" + }); + + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + var logged = Assert.Single(ReportEvents(), + e => ScalarOf(e, "Message")?.Contains(marker) == true); + Assert.Equal("app", ScalarOf(logged, "Scope")); + } + + [Fact] + public async Task Oversized_report_is_rejected_with_413_and_not_logged() + { + var client = ClientFrom("203.0.113.103"); + var marker = $"crash-{Guid.NewGuid():N}"; + + var response = await client.PostAsJsonAsync(Path, new + { + message = marker, + stack = new string('x', 64 * 1024), + scope = "screen" + }); + + Assert.Equal(HttpStatusCode.RequestEntityTooLarge, response.StatusCode); + Assert.DoesNotContain(ReportEvents(), + e => ScalarOf(e, "Message")?.Contains(marker) == true); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public async Task Blank_message_is_rejected_with_400(string message) + { + var client = ClientFrom("203.0.113.104"); + + var response = await client.PostAsJsonAsync(Path, new { message, scope = "screen" }); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + [Fact] + public async Task Unknown_scope_is_rejected_with_400() + { + var client = ClientFrom("203.0.113.105"); + + var response = await client.PostAsJsonAsync(Path, + new { message = "boom", scope = "galaxy" }); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + [Fact] + public async Task Reports_are_rate_limited_per_ip() + { + var throttled = ClientFrom("203.0.113.106"); + + for (var i = 0; i < ClientErrorReportFactory.Limit; i++) + { + var ok = await throttled.PostAsJsonAsync(Path, ValidReport()); + Assert.Equal(HttpStatusCode.Accepted, ok.StatusCode); + } + + var limited = await throttled.PostAsJsonAsync(Path, ValidReport()); + Assert.Equal(HttpStatusCode.TooManyRequests, limited.StatusCode); + Assert.True(limited.Headers.Contains("Retry-After"), + "429 must carry a Retry-After header"); + + // A different client IP still has its own budget. + var other = ClientFrom("203.0.113.107"); + var fresh = await other.PostAsJsonAsync(Path, ValidReport()); + Assert.Equal(HttpStatusCode.Accepted, fresh.StatusCode); + } + + [Fact] + public async Task Rendered_fields_are_stripped_of_control_characters_against_log_forging() + { + // The console sink renders {Message}/{Route} into a plain-text line; + // CR/LF (or ANSI escapes) from this ANONYMOUS source would let one + // report forge additional log lines. Stacks stay verbatim — they live + // in structured properties the console template never renders. + var client = ClientFrom("203.0.113.110"); + var marker = $"crash-{Guid.NewGuid():N}"; + + var response = await client.PostAsJsonAsync(Path, new + { + message = $"{marker}\r\n[00:00:00 INF] forged line \x1b[31m", + stack = "line one\nline two", + scope = "screen", + route = "/daily\nentries" + }); + + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + var logged = Assert.Single(ReportEvents(), + e => ScalarOf(e, "Message")?.Contains(marker) == true); + Assert.DoesNotContain('\n', ScalarOf(logged, "Message")!); + Assert.DoesNotContain('\r', ScalarOf(logged, "Message")!); + Assert.DoesNotContain('\x1b', ScalarOf(logged, "Message")!); + Assert.DoesNotContain('\n', ScalarOf(logged, "Route")!); + Assert.Contains("line one\nline two", ScalarOf(logged, "Stack")); + } + + [Fact] + public async Task Missing_message_field_is_rejected_with_400() + { + var client = ClientFrom("203.0.113.111"); + var response = await client.PostAsJsonAsync(Path, new { scope = "screen" }); + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + [Fact] + public async Task Non_json_body_is_rejected_with_400() + { + var client = ClientFrom("203.0.113.112"); + var response = await client.PostAsync(Path, + new StringContent("not json at all", System.Text.Encoding.UTF8, "application/json")); + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + [Fact] + public async Task Body_exactly_at_the_cap_is_accepted_and_one_byte_over_is_rejected() + { + // Probe both sides of the boundary: a report padded to EXACTLY + // MaxReportBytes must pass; one more byte must 413. + var client = ClientFrom("203.0.113.113"); + var marker = $"crash-{Guid.NewGuid():N}"; + + static string PaddedTo(string marker, int targetBytes) + { + var skeleton = $$"""{"message":"{{marker}}","scope":"screen","stack":""}"""; + return $$"""{"message":"{{marker}}","scope":"screen","stack":"{{new string('s', targetBytes - System.Text.Encoding.UTF8.GetByteCount(skeleton))}}"}"""; + } + + var atCap = PaddedTo(marker, Cluckwork.Api.Endpoints.ClientErrors.ClientErrorEndpoints.MaxReportBytes); + Assert.Equal(Cluckwork.Api.Endpoints.ClientErrors.ClientErrorEndpoints.MaxReportBytes, + System.Text.Encoding.UTF8.GetByteCount(atCap)); + var accepted = await client.PostAsync(Path, + new StringContent(atCap, System.Text.Encoding.UTF8, "application/json")); + Assert.Equal(HttpStatusCode.Accepted, accepted.StatusCode); + + var overCap = PaddedTo(marker, Cluckwork.Api.Endpoints.ClientErrors.ClientErrorEndpoints.MaxReportBytes + 1); + var rejected = await client.PostAsync(Path, + new StringContent(overCap, System.Text.Encoding.UTF8, "application/json")); + Assert.Equal(HttpStatusCode.RequestEntityTooLarge, rejected.StatusCode); + } + + [Fact] + public async Task Chunked_body_with_no_declared_length_is_still_capped() + { + // A hostile client can omit Content-Length entirely (chunked); the + // declared-length early exit never fires and the capped read loop is + // the only guard. This is the path that test must pin. + var client = ClientFrom("203.0.113.114"); + var marker = $"crash-{Guid.NewGuid():N}"; + + var oversized = $$"""{"message":"{{marker}}","scope":"screen","stack":"{{new string('s', 64 * 1024)}}"}"""; + using var request = new HttpRequestMessage(HttpMethod.Post, Path) + { + Content = new ChunkedContent(System.Text.Encoding.UTF8.GetBytes(oversized)) + }; + request.Headers.TransferEncodingChunked = true; + + var response = await client.SendAsync(request); + + Assert.Equal(HttpStatusCode.RequestEntityTooLarge, response.StatusCode); + Assert.DoesNotContain(ReportEvents(), + e => ScalarOf(e, "Message")?.Contains(marker) == true); + } + + // StringContent always computes a Content-Length; this content refuses to + // declare one, forcing the chunked/no-length path through the endpoint. + private sealed class ChunkedContent(byte[] payload) : HttpContent + { + protected override Task SerializeToStreamAsync( + Stream stream, System.Net.TransportContext? context) => + stream.WriteAsync(payload, 0, payload.Length); + + protected override bool TryComputeLength(out long length) + { + length = -1; + return false; + } + } + + [Fact] + public async Task Overlong_fields_are_truncated_in_the_log_not_rejected() + { + // Under the byte cap but over the per-field bound: the report is + // accepted and the logged property is cut, so a single pathological + // stack can't balloon one log line. + var client = ClientFrom("203.0.113.108"); + var marker = $"crash-{Guid.NewGuid():N}"; + + var response = await client.PostAsJsonAsync(Path, new + { + message = marker + new string('m', 4000), + stack = new string('s', 10_000), + scope = "screen" + }); + + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + var logged = Assert.Single(ReportEvents(), + e => ScalarOf(e, "Message")?.Contains(marker) == true); + Assert.True(ScalarOf(logged, "Message")!.Length <= 2000); + Assert.True(ScalarOf(logged, "Stack")!.Length <= 8000); + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/Infrastructure/FakeRemoteIpStartupFilter.cs b/tests/Cluckwork.Api.IntegrationTests/Infrastructure/FakeRemoteIpStartupFilter.cs new file mode 100644 index 00000000..e35d2ccc --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/Infrastructure/FakeRemoteIpStartupFilter.cs @@ -0,0 +1,24 @@ +namespace Cluckwork.Api.IntegrationTests.Infrastructure; + +using System.Net; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; + +// The in-process TestServer has no socket peer, so per-IP behavior (rate +// limiting, forwarded-header trust) has nothing to key on. This filter sets +// Connection.RemoteIpAddress from the X-Test-Remote header before the real +// pipeline (and thus before UseForwardedHeaders) runs. +public sealed class FakeRemoteIpStartupFilter : IStartupFilter +{ + public Action Configure(Action next) => app => + { + app.Use(async (ctx, nextMw) => + { + if (ctx.Request.Headers.TryGetValue("X-Test-Remote", out var remote) + && IPAddress.TryParse(remote.ToString(), out var ip)) + ctx.Connection.RemoteIpAddress = ip; + await nextMw(); + }); + next(app); + }; +} diff --git a/tests/Cluckwork.Api.IntegrationTests/RateLimitingTests.cs b/tests/Cluckwork.Api.IntegrationTests/RateLimitingTests.cs index 285d4d5e..7d240b86 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RateLimitingTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RateLimitingTests.cs @@ -32,23 +32,6 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) builder.ConfigureTestServices(services => services.AddSingleton()); } - - // Sets Connection.RemoteIpAddress from the X-Test-Remote header before the - // real pipeline (and thus before UseForwardedHeaders) runs. - private sealed class FakeRemoteIpStartupFilter : IStartupFilter - { - public Action Configure(Action next) => app => - { - app.Use(async (ctx, nextMw) => - { - if (ctx.Request.Headers.TryGetValue("X-Test-Remote", out var remote) - && IPAddress.TryParse(remote.ToString(), out var ip)) - ctx.Connection.RemoteIpAddress = ip; - await nextMw(); - }); - next(app); - }; - } } public sealed class RateLimitingTests : IClassFixture diff --git a/web/src/api/client.test.ts b/web/src/api/client.test.ts index 12a93dc9..60f9657e 100644 --- a/web/src/api/client.test.ts +++ b/web/src/api/client.test.ts @@ -14,6 +14,7 @@ import { ApiError, setOnUnauthenticated, setOnTokensChanged, + getLastTraceId, } from "./client"; import { getAccessToken, setAccessToken, clearAccessToken } from "../auth/tokenStore"; @@ -124,6 +125,48 @@ describe("apiFetch — no in-memory token", () => { }); }); +// #217 — every request mints a W3C traceparent so a browser action correlates +// with the API's request log and spans (#214). The client remembers the last +// trace id so a crash report can join the failed screen's server-side story. +describe("traceparent correlation", () => { + const TRACEPARENT = /^00-[0-9a-f]{32}-[0-9a-f]{16}-01$/; + + it("attaches a spec-shaped traceparent header to every JSON request", async () => { + fetchMock.mockResolvedValueOnce(jsonResponse({ ok: true })); + await apiGet("/stock"); + expect(headerOf(fetchMock.mock.calls[0] as Call, "traceparent")).toMatch(TRACEPARENT); + }); + + it("attaches a traceparent to blob downloads too", async () => { + // String body, not `new Blob(...)`: under vitest's jsdom env `Blob` is + // jsdom's (no .stream()) while `Response` is undici's, and whether that + // mix works depends on the Node version — it broke on CI's Node. + fetchMock.mockResolvedValueOnce(new Response("x", { status: 200 })); + await apiGetBlob("/export/eggs.csv"); + expect(headerOf(fetchMock.mock.calls[0] as Call, "traceparent")).toMatch(TRACEPARENT); + }); + + it("mints a fresh trace id per request and exposes the last one", async () => { + fetchMock + .mockResolvedValueOnce(jsonResponse({ ok: true })) + .mockResolvedValueOnce(jsonResponse({ ok: true })); + await apiGet("/stock"); + const first = headerOf(fetchMock.mock.calls[0] as Call, "traceparent")!; + await apiGet("/flocks"); + const second = headerOf(fetchMock.mock.calls[1] as Call, "traceparent")!; + + expect(second).not.toBe(first); + expect(getLastTraceId()).toBe(second.split("-")[1]); + }); + + it("remembers the trace id even when the request fails — that is the one worth reporting", async () => { + fetchMock.mockResolvedValueOnce(jsonResponse({ title: "boom" }, 500)); + await expect(apiGet("/stock")).rejects.toBeInstanceOf(ApiError); + const sent = headerOf(fetchMock.mock.calls[0] as Call, "traceparent")!; + expect(getLastTraceId()).toBe(sent.split("-")[1]); + }); +}); + describe("apiFetch — error mapping", () => { it("passes a non-401 error through without attempting a refresh", async () => { fetchMock.mockResolvedValueOnce(jsonResponse({ title: "Bad", detail: "nope" }, 400)); diff --git a/web/src/api/client.ts b/web/src/api/client.ts index d4b93035..48d63d0a 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -2,6 +2,7 @@ import type { AccessTokenResponse, LoginRequest, ProblemDetails } from "./types" import { clearAccessToken, getAccessToken, setAccessToken } from "../auth/tokenStore"; import { newId } from "../lib/ids"; import i18n from "../i18n"; +import { newTraceparent } from "../lib/traceparent"; const BASE = "/api/v1"; @@ -76,6 +77,23 @@ async function parseError(res: Response): Promise { return new ApiError(res.status, title, detail); } +// #217 — the trace id of the most recent API request, success or failure (a +// FAILED request's id is exactly the one worth having: the crash report +// attaches it, joining a browser crash to that request's server-side story). +let lastTraceId: string | null = null; +export function getLastTraceId(): string | null { + return lastTraceId; +} + +// Mint per request, never reused: the server treats the incoming id as the +// request's TraceId (#214), so sharing one across requests would fuse +// unrelated request logs into one trace. +function attachTraceparent(headers: Headers): void { + const tp = newTraceparent(); + headers.set("traceparent", tp.header); + lastTraceId = tp.traceId; +} + async function raw( path: string, init: RequestInit, @@ -86,6 +104,7 @@ async function raw( // PUTs raw image bytes, not JSON. if (!headers.has("Content-Type")) headers.set("Content-Type", "application/json"); if (accessToken) headers.set("Authorization", `Bearer ${accessToken}`); + attachTraceparent(headers); const res = await fetch(`${BASE}${path}`, { ...init, headers }); if (!res.ok) throw await parseError(res); @@ -292,9 +311,9 @@ async function rawBlob( path: string, accessToken: string, ): Promise<{ blob: Blob; filename: string | null }> { - const res = await fetch(`${BASE}${path}`, { - headers: { Authorization: `Bearer ${accessToken}` }, - }); + const headers = new Headers({ Authorization: `Bearer ${accessToken}` }); + attachTraceparent(headers); + const res = await fetch(`${BASE}${path}`, { headers }); if (!res.ok) throw await parseError(res); const disposition = res.headers.get("Content-Disposition"); const match = disposition?.match(/filename\*?=(?:UTF-8''|")?([^";]+)/i); diff --git a/web/src/api/errorReport.test.ts b/web/src/api/errorReport.test.ts new file mode 100644 index 00000000..9e465063 --- /dev/null +++ b/web/src/api/errorReport.test.ts @@ -0,0 +1,144 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { reportClientError } from "./errorReport"; +import { apiGet } from "./client"; +import { setAccessToken } from "../auth/tokenStore"; + +// #217 — the ErrorBoundary's reporting path. Best-effort by contract: whatever +// the network or server does, the returned promise RESOLVES — a reporting +// failure must never surface where it could break the fallback UI. + +type Call = [string, RequestInit]; + +let fetchMock: ReturnType; +beforeEach(() => { + fetchMock = vi.fn().mockResolvedValue(new Response(null, { status: 202 })); + vi.stubGlobal("fetch", fetchMock); +}); + +function sentBody(): Record { + const [, init] = fetchMock.mock.calls.at(-1) as Call; + return JSON.parse(init.body as string) as Record; +} + +describe("reportClientError", () => { + it("POSTs the report with keepalive so it survives an imminent reload", async () => { + await reportClientError({ + message: "kaboom", + stack: "Error: kaboom\n at Crash", + componentStack: "\n at Crash\n at ErrorBoundary", + scope: "screen", + route: "/daily-entries", + }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const [url, init] = fetchMock.mock.calls[0] as Call; + expect(url).toBe("/api/v1/client-errors"); + expect(init.method).toBe("POST"); + expect(init.keepalive).toBe(true); + expect(new Headers(init.headers).get("Content-Type")).toBe("application/json"); + expect(sentBody()).toMatchObject({ + message: "kaboom", + stack: "Error: kaboom\n at Crash", + componentStack: "\n at Crash\n at ErrorBoundary", + scope: "screen", + route: "/daily-entries", + }); + }); + + it("applies the per-field caps without shrinking when the payload already fits", async () => { + // Message over its cap, but the total stays under the byte budget: the + // message is cut to its cap and the (already-fitting) stack is untouched. + await reportClientError({ + message: "m".repeat(5000), + stack: "s".repeat(3000), + scope: "app", + }); + + const body = sentBody(); + expect((body.message as string).length).toBe(2000); + expect((body.stack as string).length).toBe(3000); + }); + + it("substitutes a placeholder when the error carries no message", async () => { + await reportClientError({ message: "", scope: "screen" }); + expect(sentBody().message).toBe("(no message)"); + }); + + it("substitutes the placeholder for a whitespace-only message the server would reject", async () => { + await reportClientError({ message: " ", scope: "screen" }); + expect(sentBody().message).toBe("(no message)"); + }); + + it("keeps the worst-case serialized payload under the server's byte cap", async () => { + // Per-field caps alone would sum past 16 KB (message 2000 + two stacks at + // 8000 + route 500 + JSON overhead) and the server would silently 413 the + // exact deep-component-tree crash this exists for. The budget is the + // SERIALIZED body, not the fields. + await reportClientError({ + message: "m".repeat(5000), + stack: "s".repeat(10_000), + componentStack: "c".repeat(10_000), + scope: "screen", + route: "/r".repeat(1000), + }); + + const [, init] = fetchMock.mock.calls.at(-1) as Call; + expect(new TextEncoder().encode(init.body as string).length).toBeLessThanOrEqual(15_000); + const body = sentBody(); + expect((body.stack as string).length).toBeGreaterThan(0); + expect(body.message as string).toContain("m"); + }); + + it("fits the byte budget even when every field is multi-byte text", async () => { + // "💥" is 4 UTF-8 bytes per glyph (2 UTF-16 code units): a char-count cap + // that fits ASCII would blow the BYTE cap here. + await reportClientError({ + message: "💥".repeat(2500), + stack: "💥".repeat(5000), + componentStack: "💥".repeat(5000), + scope: "app", + }); + + const [, init] = fetchMock.mock.calls.at(-1) as Call; + expect(new TextEncoder().encode(init.body as string).length).toBeLessThanOrEqual(15_000); + }); + + it("carries its own traceparent like every other SPA request", async () => { + await reportClientError({ message: "kaboom", scope: "screen" }); + const [, init] = fetchMock.mock.calls[0] as Call; + expect(new Headers(init.headers).get("traceparent")).toMatch( + /^00-[0-9a-f]{32}-[0-9a-f]{16}-01$/, + ); + }); + + it("includes the trace id of the last API request when one was made", async () => { + setAccessToken("token-1"); + fetchMock.mockResolvedValueOnce( + new Response("[]", { status: 200, headers: { "Content-Type": "application/json" } }), + ); + await apiGet("/flocks"); + const apiCall = fetchMock.mock.calls.find(([url]) => + (url as string).endsWith("/flocks"), + ) as Call; + const traceparent = new Headers(apiCall[1].headers).get("traceparent")!; + const traceId = traceparent.split("-")[1]; + + await reportClientError({ message: "kaboom", scope: "screen" }); + + expect(sentBody().traceId).toBe(traceId); + }); + + it("resolves when the network fails — reporting is fire-and-forget", async () => { + fetchMock.mockRejectedValue(new TypeError("network down")); + await expect( + reportClientError({ message: "kaboom", scope: "screen" }), + ).resolves.toBeUndefined(); + }); + + it("resolves when the server rejects the report (e.g. rate-limited)", async () => { + fetchMock.mockResolvedValue(new Response(null, { status: 429 })); + await expect( + reportClientError({ message: "kaboom", scope: "screen" }), + ).resolves.toBeUndefined(); + }); +}); diff --git a/web/src/api/errorReport.ts b/web/src/api/errorReport.ts new file mode 100644 index 00000000..24a53e5f --- /dev/null +++ b/web/src/api/errorReport.ts @@ -0,0 +1,76 @@ +import { getLastTraceId } from "./client"; +import { newTraceparent } from "../lib/traceparent"; + +// #217 — the ErrorBoundary's reporting path: POST the crash to the API, which +// writes it to the server log. Best-effort BY CONTRACT: the returned promise +// always resolves, whatever the network or server does — a reporting failure +// must never surface where it could break the fallback UI. +// +// Deliberately NOT apiPost: no bearer (the endpoint is anonymous — a crashing +// app may hold no usable token), no refresh-and-retry (a crash report must +// never trigger auth churn), no Idempotency-Key (nothing is stored). + +// The binding budget is the SERIALIZED body in BYTES — the server rejects +// past 16 KB and a 413'd report is silently lost, so per-field caps alone +// (which sum past the cap, and count chars, not bytes) cannot be the +// guarantee. Fields start at these sizes and every sized field shrinks by +// halves until the payload fits under 15 000 bytes (headroom for the 202 +// path's request overhead). +const MAX_BODY_BYTES = 15_000; +const MAX_MESSAGE = 2000; +const MAX_STACK = 8000; +const MAX_ROUTE = 500; + +export type ClientErrorReport = { + message: string; + stack?: string; + componentStack?: string; + scope: "app" | "screen"; + route?: string; +}; + +export function reportClientError(report: ClientErrorReport): Promise { + // Trimmed before the emptiness check: the server 400s a whitespace-only + // message, which would silently drop the report. + const message = report.message.trim(); + const build = (scale: number) => + JSON.stringify({ + message: message.slice(0, Math.floor(MAX_MESSAGE * scale)) || "(no message)", + stack: report.stack?.slice(0, Math.floor(MAX_STACK * scale)), + componentStack: report.componentStack?.slice(0, Math.floor(MAX_STACK * scale)), + scope: report.scope, + route: report.route?.slice(0, Math.floor(MAX_ROUTE * scale)), + // Set at build time (VITE_APP_VERSION); absent in dev builds. + appVersion: import.meta.env.VITE_APP_VERSION as string | undefined, + // Joins the crash to the failed screen's last API request server-side. + traceId: getLastTraceId() ?? undefined, + }); + + const byteLength = (s: string) => new TextEncoder().encode(s).length; + let scale = 1; + let body = build(scale); + // Terminates: at the floor every sized field is a handful of characters and + // the skeleton is a few hundred bytes, far under the budget. + while (byteLength(body) > MAX_BODY_BYTES && scale > 1 / 1024) { + scale /= 2; + body = build(scale); + } + + return fetch("/api/v1/client-errors", { + method: "POST", + headers: { + "Content-Type": "application/json", + // The report is an SPA API request like any other, so it carries its + // own traceparent; the PAYLOAD's traceId still points at the crashed + // screen's last request — the causal one. + traceparent: newTraceparent().header, + }, + body, + // The common crash follow-up is the fallback's Reload button; keepalive + // lets the report finish through the navigation. + keepalive: true, + }).then( + () => undefined, + () => undefined, + ); +} diff --git a/web/src/components/ErrorBoundary.test.tsx b/web/src/components/ErrorBoundary.test.tsx index 987972cd..d9fe0f09 100644 --- a/web/src/components/ErrorBoundary.test.tsx +++ b/web/src/components/ErrorBoundary.test.tsx @@ -163,4 +163,56 @@ describe("ErrorBoundary", () => { expect.anything(), ); }); + + // #217 — a caught crash is reported to the API so the operator learns a + // screen is crashing without a support screenshot. + describe("crash reporting", () => { + let reportMock: ReturnType; + beforeEach(() => { + reportMock = vi.fn().mockResolvedValue(new Response(null, { status: 202 })); + vi.stubGlobal("fetch", reportMock); + }); + + const reportCalls = () => + reportMock.mock.calls.filter(([url]) => (url as string).endsWith("/client-errors")); + + it("POSTs one report with the message, stacks, scope and route", () => { + inRouter( + + + , + ); + const calls = reportCalls(); + expect(calls).toHaveLength(1); + const body = JSON.parse((calls[0][1] as RequestInit).body as string) as Record; + expect(body.message).toBe("report me"); + expect(body.scope).toBe("screen"); + expect(body.route).toBe(window.location.pathname); + expect(body.stack).toEqual(expect.stringContaining("report me")); + expect(body.componentStack).toEqual(expect.any(String)); + }); + + it("reports the app scope from the app boundary", () => { + render( + + + , + ); + const body = JSON.parse((reportCalls()[0][1] as RequestInit).body as string) as Record; + expect(body.scope).toBe("app"); + }); + + it("still renders the fallback when the report itself fails", async () => { + reportMock.mockRejectedValue(new TypeError("network down")); + inRouter( + + + , + ); + expect(screen.getByText("Something went wrong")).toBeInTheDocument(); + // Let the rejected report settle — it must be swallowed, not unhandled. + await new Promise((r) => setTimeout(r, 0)); + expect(screen.getByText("Something went wrong")).toBeInTheDocument(); + }); + }); }); diff --git a/web/src/components/ErrorBoundary.tsx b/web/src/components/ErrorBoundary.tsx index 9f54b197..b2eb2db5 100644 --- a/web/src/components/ErrorBoundary.tsx +++ b/web/src/components/ErrorBoundary.tsx @@ -1,5 +1,6 @@ import { Component, useEffect, useRef, type ErrorInfo, type ReactNode } from "react"; import { Link } from "react-router"; +import { reportClientError } from "../api/errorReport"; // #140: without a boundary, any throw during render unmounts the whole tree and // the user is left on a blank page — no message, no way back. On a phone in a @@ -42,6 +43,18 @@ export class ErrorBoundary extends Component { // Reachable in the console and logs for a support screenshot, without // shouting the stack at the user on the page. console.error("Render error caught by boundary:", error, info.componentStack); + // #217 — best-effort report to the operator's log. Fire-and-forget: the + // promise always resolves (errorReport swallows every failure), so nothing + // here can break or delay the fallback UI already rendering. + void reportClientError({ + message: error.message, + stack: error.stack, + componentStack: info.componentStack ?? undefined, + scope: this.props.scope, + // pathname only: the query string can carry user-typed values, and the + // route's job in the log is "which screen", not "which filters". + route: window.location.pathname, + }); } render() { diff --git a/web/src/lib/traceparent.test.ts b/web/src/lib/traceparent.test.ts new file mode 100644 index 00000000..4b216e45 --- /dev/null +++ b/web/src/lib/traceparent.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { newTraceparent } from "./traceparent"; + +// #217 — every API request carries a W3C traceparent minted here, so a browser +// action correlates with the API's request log and spans (#214). Format: +// 00-{32 hex trace-id}-{16 hex parent-id}-{flags}; an all-zero trace-id or +// parent-id is INVALID per the spec and a receiver may discard the header. + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("newTraceparent", () => { + it("mints a spec-shaped header with the trace id exposed separately", () => { + const tp = newTraceparent(); + expect(tp.header).toMatch(/^00-[0-9a-f]{32}-[0-9a-f]{16}-01$/); + expect(tp.header.split("-")[1]).toBe(tp.traceId); + }); + + it("mints a different trace id per call", () => { + expect(newTraceparent().traceId).not.toBe(newTraceparent().traceId); + }); + + it("never emits the all-zero ids the spec declares invalid", () => { + // Force the RNG to hand back zeros once per segment; the generator must + // retry rather than emit 00000000000000000000000000000000. + const real = crypto.getRandomValues.bind(crypto); + let zeroCalls = 2; // first trace-id draw + first parent-id draw + vi.stubGlobal("crypto", { + getRandomValues: (buf: Uint8Array) => { + if (zeroCalls > 0) { + zeroCalls--; + buf.fill(0); + return buf; + } + return real(buf); + }, + }); + + const tp = newTraceparent(); + expect(tp.traceId).not.toBe("0".repeat(32)); + expect(tp.header.split("-")[2]).not.toBe("0".repeat(16)); + expect(tp.header).toMatch(/^00-[0-9a-f]{32}-[0-9a-f]{16}-01$/); + }); +}); diff --git a/web/src/lib/traceparent.ts b/web/src/lib/traceparent.ts new file mode 100644 index 00000000..37e5de42 --- /dev/null +++ b/web/src/lib/traceparent.ts @@ -0,0 +1,25 @@ +// #217 — W3C Trace Context (https://www.w3.org/TR/trace-context/). The SPA +// mints a `traceparent` per API request so a browser-initiated action +// correlates with the API's request log and exported spans (#214): the id the +// browser sent IS the TraceId on the server's completion event. ASP.NET Core +// honors the header natively; the API's AlwaysOnSampler means the flags byte +// cannot suppress server-side sampling either way. + +export type Traceparent = { header: string; traceId: string }; + +export function newTraceparent(): Traceparent { + const traceId = randomNonZeroHex(16); + const parentId = randomNonZeroHex(8); + return { header: `00-${traceId}-${parentId}-01`, traceId }; +} + +// An all-zero trace-id or parent-id is the spec's "invalid" sentinel — a +// receiver may discard the whole header. getRandomValues handing back all +// zeros is astronomically unlikely, but the guard is one loop check. +function randomNonZeroHex(byteLength: number): string { + const buf = new Uint8Array(byteLength); + do { + crypto.getRandomValues(buf); + } while (buf.every((b) => b === 0)); + return Array.from(buf, (b) => b.toString(16).padStart(2, "0")).join(""); +} diff --git a/web/vite.config.ts b/web/vite.config.ts index 198870d2..6c98bb23 100644 --- a/web/vite.config.ts +++ b/web/vite.config.ts @@ -71,6 +71,11 @@ export default defineConfig(({ mode }) => { // performance win. #50 adds an explicit, deliberate offline path. runtimeCaching: [], navigationPreload: false, + // #217 — the app's own maps are emitted hidden (see build below); + // workbox would otherwise ship sw.js.map + its own map WITH + // sourceMappingURL comments. They only cover generated worker + // code, so drop them rather than special-case them. + sourcemap: false, }, // The SW is a production artifact; leaving it off in dev keeps `npm run // dev` free of stale-cache confusion. @@ -83,6 +88,14 @@ export default defineConfig(({ mode }) => { "/api": { target, changeOrigin: true }, }, }, + // #217 — "hidden": emit .map files so a reported minified stack resolves + // to source lines, but without the sourceMappingURL comment, so browsers + // never fetch them uninvited. They ship next to the bundles; resolving a + // stack is an operator action (source-map CLI or a browser devtools "add + // source map"), not something the public page advertises. + build: { + sourcemap: "hidden", + }, // Unit tests only (Vitest). E2E stays the manual Playwright drill (#105). // Explicit vitest imports in each test — no globals — so the app's strict // tsconfig stays clean of test-runner ambient types.