diff --git a/NextAurora.ServiceDefaults/Extensions.cs b/NextAurora.ServiceDefaults/Extensions.cs index ed4b725b..7f803d74 100644 --- a/NextAurora.ServiceDefaults/Extensions.cs +++ b/NextAurora.ServiceDefaults/Extensions.cs @@ -134,9 +134,24 @@ public static TBuilder AddDefaultHealthChecks(this TBuilder builder) w public static WebApplication MapDefaultEndpoints(this WebApplication app) { - app.UseMiddleware(); + // ORDER MATTERS. CorrelationIdMiddleware sits BETWEEN UseAuthentication and + // UseAuthorization for two reasons: + // + // 1. It reads ClaimTypes.NameIdentifier from context.User to populate the + // UserId scope key. UseAuthentication populates context.User, so the + // middleware must run AFTER it. Otherwise UserId is silently always null. + // + // 2. Placing it BEFORE UseAuthorization (rather than after) means the + // UserId scope is active during the Authorization step itself. Any + // 401/403 denials get logged with the authenticated user's ID — + // preserving the audit trail for "user X tried to access resource they + // shouldn't." Running after Authorization would drop that signal, + // losing visibility into unauthorized-attempt patterns. + // + // UseExceptionHandler stays first so it wraps every error below. app.UseExceptionHandler(); app.UseAuthentication(); + app.UseMiddleware(); app.UseAuthorization(); // All health checks must pass for app to be considered ready to accept traffic after starting @@ -176,6 +191,16 @@ private static TBuilder AddDefaultAuthentication(this TBuilder builder ValidateAudience = true, ValidateIssuer = true, ValidateLifetime = true, + // Explicit — JWT Bearer's implicit default already validates the + // signature against JWKS-discovered keys, but making it explicit + // makes the security posture auditable + prevents a future config + // change from accidentally disabling signature validation. + ValidateIssuerSigningKey = true, + // Default ClockSkew is 5 minutes — revoked/expired tokens remain + // accepted for 5 extra minutes, which is material on a 15-minute + // access-token lifetime. 30 seconds covers reasonable inter-server + // clock drift without giving attackers a long replay window. + ClockSkew = TimeSpan.FromSeconds(30), NameClaimType = "preferred_username", RoleClaimType = "realm_access.roles", }; diff --git a/NextAurora.ServiceDefaults/GlobalExceptionHandler.cs b/NextAurora.ServiceDefaults/GlobalExceptionHandler.cs index bd64c7cb..e29d8f32 100644 --- a/NextAurora.ServiceDefaults/GlobalExceptionHandler.cs +++ b/NextAurora.ServiceDefaults/GlobalExceptionHandler.cs @@ -13,7 +13,13 @@ public sealed class GlobalExceptionHandler(ILogger logge public async ValueTask TryHandleAsync(HttpContext httpContext, Exception exception, CancellationToken cancellationToken) { - var traceId = Activity.Current?.Id ?? httpContext.TraceIdentifier; + // Use TraceId, NOT Activity.Current?.Id. Activity.Id is the full W3C + // traceparent ("00---") — returning that to clients + // leaks the span ID, which is information about server-side handler call + // structure that clients have no business seeing. Trace ID alone is the + // correlation token clients legitimately need. See CLAUDE.md "Security + // Requirements — Error Handling: Never expose internal state". + var traceId = Activity.Current?.TraceId.ToString() ?? httpContext.TraceIdentifier; logger.LogError(exception, "Unhandled exception occurred. TraceId: {TraceId}", traceId); diff --git a/tests/OrderService.Tests.Unit/Application/GlobalExceptionHandlerTests.cs b/tests/OrderService.Tests.Unit/Application/GlobalExceptionHandlerTests.cs new file mode 100644 index 00000000..875ec443 --- /dev/null +++ b/tests/OrderService.Tests.Unit/Application/GlobalExceptionHandlerTests.cs @@ -0,0 +1,100 @@ +using System.Diagnostics; +using System.Text.Json; +using AwesomeAssertions; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging; +using NextAurora.ServiceDefaults; +using NSubstitute; + +namespace OrderService.Tests.Unit.Application; + +/// +/// Tests for — specifically the recent change +/// that switched the response traceId from Activity.Current?.Id (full +/// W3C traceparent with span ID) to Activity.Current?.TraceId.ToString() +/// (trace component only). The span-ID leak is a CWE-200 information disclosure +/// of server-side handler call structure; tests pin the correct shape. +/// +public class GlobalExceptionHandlerTests +{ + private readonly ILogger _logger = + Substitute.For>(); + + [Fact] + public async Task TryHandleAsync_WhenActivityIsActive_ReturnsTraceIdOnly_NotFullActivityId() + { + using var activity = new Activity("test"); + activity.Start(); + var (sut, httpContext, responseStream) = BuildSut(); + + var handled = await sut.TryHandleAsync( + httpContext, new InvalidOperationException("boom"), CancellationToken.None); + + handled.Should().BeTrue(); + var traceIdInResponse = await ExtractTraceIdAsync(responseStream); + + // The response traceId must be the trace component only (32 hex chars), NOT + // Activity.Id which is the full W3C traceparent "00---" + // and leaks span information. + traceIdInResponse.Should().Be(activity.TraceId.ToString()); + traceIdInResponse.Should().NotBe(activity.Id); + traceIdInResponse.Should().NotContain("-", + "the W3C traceparent format uses hyphens to separate version/trace/span/flags; the bare trace component has none"); + } + + [Fact] + public async Task TryHandleAsync_WhenNoActivity_FallsBackToHttpContextTraceIdentifier() + { + // Belt-and-suspenders: ensure no Activity is leaking from a previous test. + // Activity.Current is an AsyncLocal; in xunit a class's tests run sequentially + // so the `using` in the prior test should have stopped its activity, but + // assert here to be explicit. + Activity.Current.Should().BeNull(); + + var (sut, httpContext, responseStream) = BuildSut(); + httpContext.TraceIdentifier = "fallback-trace-from-aspnetcore"; + + await sut.TryHandleAsync( + httpContext, new InvalidOperationException("boom"), CancellationToken.None); + + var traceIdInResponse = await ExtractTraceIdAsync(responseStream); + traceIdInResponse.Should().Be("fallback-trace-from-aspnetcore"); + } + + [Fact] + public async Task TryHandleAsync_AlwaysReturnsProblemDetailsWithGenericDetail_NeverExceptionMessage() + { + // Defense against information disclosure: the response must never include the + // raw exception message (which can leak SQL fragments, file paths, stack + // traces, etc.). Per CLAUDE.md "Security Requirements — Error Handling". + var (sut, httpContext, responseStream) = BuildSut(); + var sensitiveException = new InvalidOperationException( + "SELECT * FROM Users WHERE Password = 'super-secret-leaked-via-error'"); + + await sut.TryHandleAsync(httpContext, sensitiveException, CancellationToken.None); + + responseStream.Position = 0; + using var reader = new StreamReader(responseStream); + var body = await reader.ReadToEndAsync(); + body.Should().NotContain("super-secret-leaked-via-error"); + body.Should().NotContain("SELECT * FROM"); + } + + private (GlobalExceptionHandler sut, DefaultHttpContext context, MemoryStream responseStream) BuildSut() + { + var sut = new GlobalExceptionHandler(_logger); + var httpContext = new DefaultHttpContext(); + var responseStream = new MemoryStream(); + httpContext.Response.Body = responseStream; + return (sut, httpContext, responseStream); + } + + private static async Task ExtractTraceIdAsync(MemoryStream responseStream) + { + responseStream.Position = 0; + using var reader = new StreamReader(responseStream); + var body = await reader.ReadToEndAsync(); + var problem = JsonSerializer.Deserialize(body); + return problem.GetProperty("traceId").GetString(); + } +} diff --git a/tests/OrderService.Tests.Unit/Application/ServiceDefaultsJwtTests.cs b/tests/OrderService.Tests.Unit/Application/ServiceDefaultsJwtTests.cs new file mode 100644 index 00000000..875cabb0 --- /dev/null +++ b/tests/OrderService.Tests.Unit/Application/ServiceDefaultsJwtTests.cs @@ -0,0 +1,78 @@ +using AwesomeAssertions; +using Microsoft.AspNetCore.Authentication.JwtBearer; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Options; + +namespace OrderService.Tests.Unit.Application; + +/// +/// Tests for the JWT Bearer options configured in +/// NextAurora.ServiceDefaults.Extensions.AddDefaultAuthentication — pins the +/// security-hardened defaults (explicit signing-key validation + tight ClockSkew) +/// so a future config change can't silently weaken them. +/// +public class ServiceDefaultsJwtTests +{ + [Fact] + public void AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation() + { + using var host = BuildHostWithAuthority(); + var options = host.Services + .GetRequiredService>() + .Get(JwtBearerDefaults.AuthenticationScheme); + + // Without this, JWT Bearer's implicit default still validates (via JWKS), but + // making it explicit makes the security posture auditable + prevents a future + // config refactor from accidentally disabling signature validation. + options.TokenValidationParameters.ValidateIssuerSigningKey.Should().BeTrue(); + } + + [Fact] + public void AddServiceDefaults_WhenAuthorityConfigured_SetsTightClockSkew() + { + using var host = BuildHostWithAuthority(); + var options = host.Services + .GetRequiredService>() + .Get(JwtBearerDefaults.AuthenticationScheme); + + // Default ClockSkew is 5 minutes — revoked/expired tokens stay accepted for + // 5 extra minutes, material on typical 15-minute access-token lifetimes. We + // pin 30 seconds: covers inter-server clock drift without giving attackers a + // long replay window. + options.TokenValidationParameters.ClockSkew.Should().Be(TimeSpan.FromSeconds(30)); + } + + [Fact] + public void AddServiceDefaults_WhenAuthorityConfigured_RetainsCoreValidations() + { + // Defense against a refactor that drops one of the foundational validations. + // ValidateAudience/Issuer/Lifetime were already on; this test pins them. + using var host = BuildHostWithAuthority(); + var options = host.Services + .GetRequiredService>() + .Get(JwtBearerDefaults.AuthenticationScheme); + + options.TokenValidationParameters.ValidateAudience.Should().BeTrue(); + options.TokenValidationParameters.ValidateIssuer.Should().BeTrue(); + options.TokenValidationParameters.ValidateLifetime.Should().BeTrue(); + } + + private static IHost BuildHostWithAuthority() + { + var builder = Host.CreateApplicationBuilder(); + + // AddDefaultAuthentication branches on whether an authority is configured. + // With one, it wires AddJwtBearer with the TokenValidationParameters we're + // testing. Without one, it registers no-op auth and the JwtBearerOptions + // doesn't get configured. + builder.Configuration.AddInMemoryCollection(new Dictionary(StringComparer.Ordinal) + { + ["Keycloak:Url"] = "https://example.keycloak.test/realms/nextaurora", + }); + + builder.AddServiceDefaults(); + return builder.Build(); + } +}