fix(service-defaults): security trio — middleware order, JWT, error t…

NextAurora.ServiceDefaults/Extensions.cs

@@ -134,9 +134,24 @@ public static TBuilder AddDefaultHealthChecks<TBuilder>(this TBuilder builder) w
 
     public static WebApplication MapDefaultEndpoints(this WebApplication app)
     {
-        app.UseMiddleware<CorrelationIdMiddleware>();
+        // 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<CorrelationIdMiddleware>();
         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<TBuilder>(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",
                 };

NextAurora.ServiceDefaults/GlobalExceptionHandler.cs

@@ -13,7 +13,13 @@ public sealed class GlobalExceptionHandler(ILogger<GlobalExceptionHandler> logge
 
     public async ValueTask<bool> 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-<trace>-<span>-<flags>") — 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);
 

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;
+
+/// <summary>
+/// Tests for <see cref="GlobalExceptionHandler"/> — specifically the recent change
+/// that switched the response <c>traceId</c> from <c>Activity.Current?.Id</c> (full
+/// W3C traceparent with span ID) to <c>Activity.Current?.TraceId.ToString()</c>
+/// (trace component only). The span-ID leak is a CWE-200 information disclosure
+/// of server-side handler call structure; tests pin the correct shape.
+/// </summary>
+public class GlobalExceptionHandlerTests
+{
+    private readonly ILogger<GlobalExceptionHandler> _logger =
+        Substitute.For<ILogger<GlobalExceptionHandler>>();
+
+    [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-<trace>-<span>-<flags>"
+        // 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<string?> ExtractTraceIdAsync(MemoryStream responseStream)
+    {
+        responseStream.Position = 0;
+        using var reader = new StreamReader(responseStream);
+        var body = await reader.ReadToEndAsync();
+        var problem = JsonSerializer.Deserialize<JsonElement>(body);
+        return problem.GetProperty("traceId").GetString();
+    }
+}

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;
+
+/// <summary>
+/// Tests for the JWT Bearer options configured in
+/// <c>NextAurora.ServiceDefaults.Extensions.AddDefaultAuthentication</c> — pins the
+/// security-hardened defaults (explicit signing-key validation + tight ClockSkew)
+/// so a future config change can't silently weaken them.
+/// </summary>
+public class ServiceDefaultsJwtTests
+{
+    [Fact]
+    public void AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation()
+    {
+        using var host = BuildHostWithAuthority();
+        var options = host.Services
+            .GetRequiredService<IOptionsMonitor<JwtBearerOptions>>()
+            .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<IOptionsMonitor<JwtBearerOptions>>()
+            .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<IOptionsMonitor<JwtBearerOptions>>()
+            .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<string, string?>(StringComparer.Ordinal)
+        {
+            ["Keycloak:Url"] = "https://example.keycloak.test/realms/nextaurora",
+        });
+
+        builder.AddServiceDefaults();
+        return builder.Build();
+    }
+}