diff --git a/SatelliteProvider.Api/CorsConfigurationValidator.cs b/SatelliteProvider.Api/CorsConfigurationValidator.cs new file mode 100644 index 0000000..fb27f9c --- /dev/null +++ b/SatelliteProvider.Api/CorsConfigurationValidator.cs @@ -0,0 +1,41 @@ +namespace SatelliteProvider.Api; + +public static class CorsConfigurationValidator +{ + public const string MissingOriginsMessage = + "CORS is misconfigured: CorsConfig:AllowedOrigins is empty and CorsConfig:AllowAnyOrigin is not true. " + + "Refusing to start in Production with a permissive CORS policy. " + + "Set CorsConfig:AllowedOrigins to a non-empty array, or set CorsConfig:AllowAnyOrigin=true to opt in."; + + public const string PermissiveDefaultWarning = + "CorsConfig:AllowedOrigins is empty and CorsConfig:AllowAnyOrigin is not true. " + + "Permissive CORS is being applied for environment {Environment}; do not run with this configuration in Production."; + + public static void EnsureSafeForEnvironment( + string[] allowedOrigins, + bool allowAnyOrigin, + string environmentName) + { + ArgumentNullException.ThrowIfNull(allowedOrigins); + ArgumentNullException.ThrowIfNull(environmentName); + + if (allowedOrigins.Length == 0 + && !allowAnyOrigin + && string.Equals(environmentName, "Production", StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException(MissingOriginsMessage); + } + } + + public static bool ShouldUsePermissivePolicy(string[] allowedOrigins, bool allowAnyOrigin) + { + ArgumentNullException.ThrowIfNull(allowedOrigins); + return allowAnyOrigin || allowedOrigins.Length == 0; + } + + public static bool ShouldWarnAboutPermissiveDefault(string[] allowedOrigins, bool allowAnyOrigin) + { + ArgumentNullException.ThrowIfNull(allowedOrigins); + return allowedOrigins.Length == 0 && !allowAnyOrigin; + } +} diff --git a/SatelliteProvider.Api/GlobalExceptionHandler.cs b/SatelliteProvider.Api/GlobalExceptionHandler.cs new file mode 100644 index 0000000..b5be05f --- /dev/null +++ b/SatelliteProvider.Api/GlobalExceptionHandler.cs @@ -0,0 +1,76 @@ +using Microsoft.AspNetCore.Diagnostics; +using Microsoft.AspNetCore.Mvc; + +namespace SatelliteProvider.Api; + +public sealed class GlobalExceptionHandler : IExceptionHandler +{ + private readonly ILogger _logger; + + public GlobalExceptionHandler(ILogger logger) + { + _logger = logger; + } + + public async ValueTask TryHandleAsync( + HttpContext httpContext, + Exception exception, + CancellationToken cancellationToken) + { + // Framework-level request-binding/parsing failures carry their own HTTP status + // (typically 400/415). Honor that status so we don't promote a client error to 5xx. + if (exception is BadHttpRequestException badRequest) + { + await WriteClientErrorAsync(httpContext, badRequest, cancellationToken); + return true; + } + + var correlationId = httpContext.TraceIdentifier; + + _logger.LogError( + exception, + "Unhandled exception while processing {Method} {Path} (correlationId={CorrelationId})", + httpContext.Request.Method, + httpContext.Request.Path, + correlationId); + + httpContext.Response.StatusCode = StatusCodes.Status500InternalServerError; + + var problem = new ProblemDetails + { + Status = StatusCodes.Status500InternalServerError, + Title = "Internal Server Error", + Detail = "An unexpected error occurred. Use the correlationId to look up the server log entry.", + Type = "https://datatracker.ietf.org/doc/html/rfc9110#name-500-internal-server-error", + }; + problem.Extensions["correlationId"] = correlationId; + + await httpContext.Response.WriteAsJsonAsync( + problem, + options: null, + contentType: "application/problem+json", + cancellationToken: cancellationToken); + return true; + } + + private static async Task WriteClientErrorAsync( + HttpContext httpContext, + BadHttpRequestException badRequest, + CancellationToken cancellationToken) + { + httpContext.Response.StatusCode = badRequest.StatusCode; + + var problem = new ProblemDetails + { + Status = badRequest.StatusCode, + Title = "Bad Request", + Detail = badRequest.Message, + }; + + await httpContext.Response.WriteAsJsonAsync( + problem, + options: null, + contentType: "application/problem+json", + cancellationToken: cancellationToken); + } +} diff --git a/SatelliteProvider.Api/Program.cs b/SatelliteProvider.Api/Program.cs index 20cb4c5..7bc7a40 100644 --- a/SatelliteProvider.Api/Program.cs +++ b/SatelliteProvider.Api/Program.cs @@ -2,6 +2,7 @@ using System.ComponentModel.DataAnnotations; using Microsoft.AspNetCore.Mvc; using Microsoft.OpenApi.Models; using Swashbuckle.AspNetCore.SwaggerGen; +using SatelliteProvider.Api; using SatelliteProvider.DataAccess; using SatelliteProvider.DataAccess.Repositories; using SatelliteProvider.Common.Configs; @@ -35,17 +36,23 @@ builder.Services.AddRegionProcessing(); builder.Services.AddRouteManagement(); var allowedOrigins = builder.Configuration.GetSection("CorsConfig:AllowedOrigins").Get() ?? Array.Empty(); +var allowAnyOrigin = builder.Configuration.GetValue("CorsConfig:AllowAnyOrigin"); +CorsConfigurationValidator.EnsureSafeForEnvironment(allowedOrigins, allowAnyOrigin, builder.Environment.EnvironmentName); + builder.Services.AddCors(options => { options.AddPolicy("TilesCors", policy => { - if (allowedOrigins.Length > 0) - policy.WithOrigins(allowedOrigins).AllowAnyHeader().AllowAnyMethod(); - else + if (CorsConfigurationValidator.ShouldUsePermissivePolicy(allowedOrigins, allowAnyOrigin)) policy.AllowAnyOrigin().AllowAnyHeader().AllowAnyMethod(); + else + policy.WithOrigins(allowedOrigins).AllowAnyHeader().AllowAnyMethod(); }); }); +builder.Services.AddProblemDetails(); +builder.Services.AddExceptionHandler(); + builder.Services.ConfigureHttpJsonOptions(options => { options.SerializerOptions.PropertyNamingPolicy = System.Text.Json.JsonNamingPolicy.CamelCase; @@ -79,6 +86,13 @@ builder.Services.AddSwaggerGen(c => var app = builder.Build(); +if (CorsConfigurationValidator.ShouldWarnAboutPermissiveDefault(allowedOrigins, allowAnyOrigin)) +{ + app.Services + .GetRequiredService>() + .LogWarning(CorsConfigurationValidator.PermissiveDefaultWarning, app.Environment.EnvironmentName); +} + var migratorLogger = app.Services.GetRequiredService>(); var migrator = new DatabaseMigrator(connectionString, migratorLogger); if (!migrator.RunMigrations()) @@ -96,6 +110,7 @@ if (app.Environment.IsDevelopment()) app.UseSwaggerUI(); } +app.UseExceptionHandler(); app.UseHttpsRedirection(); app.UseCors("TilesCors"); @@ -106,11 +121,13 @@ app.MapGet("/api/satellite/tiles/latlon", GetTileByLatLon) .WithOpenApi(op => new(op) { Summary = "Get satellite tile by latitude and longitude coordinates" }); app.MapGet("/api/satellite/tiles/mgrs", GetSatelliteTilesByMgrs) - .WithOpenApi(op => new(op) { Summary = "Get satellite tiles by MGRS coordinates" }); + .ProducesProblem(StatusCodes.Status501NotImplemented) + .WithOpenApi(op => new(op) { Summary = "Get satellite tiles by MGRS coordinates (NOT IMPLEMENTED)" }); app.MapPost("/api/satellite/upload", UploadImage) .Accepts("multipart/form-data") - .WithOpenApi(op => new(op) { Summary = "Upload image with metadata and save to /maps folder" }) + .ProducesProblem(StatusCodes.Status501NotImplemented) + .WithOpenApi(op => new(op) { Summary = "Upload image with metadata (NOT IMPLEMENTED)" }) .DisableAntiforgery(); app.MapPost("/api/satellite/request", RequestRegion) @@ -127,107 +144,81 @@ app.MapGet("/api/satellite/route/{id:guid}", GetRoute) app.Run(); -async Task ServeTile(int z, int x, int y, HttpContext httpContext, ITileService tileService, ILogger logger) +async Task ServeTile(int z, int x, int y, HttpContext httpContext, ITileService tileService) { - try - { - var tile = await tileService.GetOrDownloadTileAsync(z, x, y, httpContext.RequestAborted); - httpContext.Response.Headers.CacheControl = $"public, max-age={(long)tile.MaxAge.TotalSeconds}"; - httpContext.Response.Headers.ETag = tile.ETag; - return Results.Bytes(tile.Bytes, tile.ContentType); - } - catch (Exception ex) - { - logger.LogError(ex, "Failed to serve tile {Z}/{X}/{Y}", z, x, y); - return Results.Problem(detail: ex.Message, statusCode: 500); - } + var tile = await tileService.GetOrDownloadTileAsync(z, x, y, httpContext.RequestAborted); + httpContext.Response.Headers.CacheControl = $"public, max-age={(long)tile.MaxAge.TotalSeconds}"; + httpContext.Response.Headers.ETag = tile.ETag; + return Results.Bytes(tile.Bytes, tile.ContentType); } -async Task GetTileByLatLon([FromQuery] double Latitude, [FromQuery] double Longitude, [FromQuery] int ZoomLevel, ITileService tileService, ILogger logger) +async Task GetTileByLatLon([FromQuery] double Latitude, [FromQuery] double Longitude, [FromQuery] int ZoomLevel, ITileService tileService) { - try - { - var tile = await tileService.DownloadAndStoreSingleTileAsync(Latitude, Longitude, ZoomLevel); + var tile = await tileService.DownloadAndStoreSingleTileAsync(Latitude, Longitude, ZoomLevel); - var response = new DownloadTileResponse - { - Id = tile.Id, - ZoomLevel = tile.TileZoom, - Latitude = tile.Latitude, - Longitude = tile.Longitude, - TileSizeMeters = tile.TileSizeMeters, - TileSizePixels = tile.TileSizePixels, - ImageType = tile.ImageType, - MapsVersion = tile.MapsVersion, - Version = tile.Version, - FilePath = tile.FilePath, - CreatedAt = tile.CreatedAt, - UpdatedAt = tile.UpdatedAt - }; - - return Results.Ok(response); - } - catch (Exception ex) + var response = new DownloadTileResponse { - logger.LogError(ex, "Failed to get tile"); - return Results.Problem(detail: ex.Message, statusCode: 500); - } + Id = tile.Id, + ZoomLevel = tile.TileZoom, + Latitude = tile.Latitude, + Longitude = tile.Longitude, + TileSizeMeters = tile.TileSizeMeters, + TileSizePixels = tile.TileSizePixels, + ImageType = tile.ImageType, + MapsVersion = tile.MapsVersion, + Version = tile.Version, + FilePath = tile.FilePath, + CreatedAt = tile.CreatedAt, + UpdatedAt = tile.UpdatedAt + }; + + return Results.Ok(response); } IResult GetSatelliteTilesByMgrs(string mgrs, double squareSideMeters) { - return Results.Ok(new GetSatelliteTilesResponse()); + return Results.Problem( + statusCode: StatusCodes.Status501NotImplemented, + title: "Not implemented", + detail: "MGRS-based tile retrieval is not implemented."); } IResult UploadImage([FromForm] UploadImageRequest request) { - return Results.Ok(new SaveResult { Success = false }); + return Results.Problem( + statusCode: StatusCodes.Status501NotImplemented, + title: "Not implemented", + detail: "Image upload is not implemented."); } -async Task RequestRegion([FromBody] RequestRegionRequest request, IRegionService regionService, ILogger logger) +async Task RequestRegion([FromBody] RequestRegionRequest request, IRegionService regionService) { - try + if (request.SizeMeters < 100 || request.SizeMeters > 10000) { - if (request.SizeMeters < 100 || request.SizeMeters > 10000) - { - return Results.BadRequest(new { error = "Size must be between 100 and 10000 meters" }); - } - - var status = await regionService.RequestRegionAsync( - request.Id, - request.Latitude, - request.Longitude, - request.SizeMeters, - request.ZoomLevel, - request.StitchTiles); - - return Results.Ok(status); - } - catch (Exception ex) - { - logger.LogError(ex, "Failed to request region"); - return Results.Problem(detail: ex.Message, statusCode: 500); + return Results.BadRequest(new { error = "Size must be between 100 and 10000 meters" }); } + + var status = await regionService.RequestRegionAsync( + request.Id, + request.Latitude, + request.Longitude, + request.SizeMeters, + request.ZoomLevel, + request.StitchTiles); + + return Results.Ok(status); } -async Task GetRegionStatus(Guid id, IRegionService regionService, ILogger logger) +async Task GetRegionStatus(Guid id, IRegionService regionService) { - try - { - var status = await regionService.GetRegionStatusAsync(id); - - if (status == null) - { - return Results.NotFound(new { error = $"Region {id} not found" }); - } + var status = await regionService.GetRegionStatusAsync(id); - return Results.Ok(status); - } - catch (Exception ex) + if (status == null) { - logger.LogError(ex, "Failed to get region status"); - return Results.Problem(detail: ex.Message, statusCode: 500); + return Results.NotFound(new { error = $"Region {id} not found" }); } + + return Results.Ok(status); } async Task CreateRoute([FromBody] CreateRouteRequest request, IRouteService routeService, ILogger logger) @@ -242,31 +233,18 @@ async Task CreateRoute([FromBody] CreateRouteRequest request, IRouteSer logger.LogWarning(ex, "Invalid route request"); return Results.BadRequest(new { error = ex.Message }); } - catch (Exception ex) - { - logger.LogError(ex, "Failed to create route"); - return Results.Problem(detail: ex.Message, statusCode: 500); - } } -async Task GetRoute(Guid id, IRouteService routeService, ILogger logger) +async Task GetRoute(Guid id, IRouteService routeService) { - try - { - var route = await routeService.GetRouteAsync(id); - - if (route == null) - { - return Results.NotFound(new { error = $"Route {id} not found" }); - } + var route = await routeService.GetRouteAsync(id); - return Results.Ok(route); - } - catch (Exception ex) + if (route == null) { - logger.LogError(ex, "Failed to get route"); - return Results.Problem(detail: ex.Message, statusCode: 500); + return Results.NotFound(new { error = $"Route {id} not found" }); } + + return Results.Ok(route); } public record GetSatelliteTilesResponse diff --git a/SatelliteProvider.IntegrationTests/Program.cs b/SatelliteProvider.IntegrationTests/Program.cs index 8942fe9..75c020b 100644 --- a/SatelliteProvider.IntegrationTests/Program.cs +++ b/SatelliteProvider.IntegrationTests/Program.cs @@ -67,6 +67,7 @@ class Program await BasicRouteTests.RunSimpleRouteTest(httpClient); await ExtendedRouteTests.RunRouteWithTilesZipTest(httpClient); await SecurityTests.RunAll(httpClient); + await StubAndErrorContractTests.RunAll(httpClient); } static async Task RunFullSuite(HttpClient httpClient) @@ -85,6 +86,7 @@ class Program await ExtendedRouteTests.RunExtendedRouteEast(httpClient); await SecurityTests.RunAll(httpClient); + await StubAndErrorContractTests.RunAll(httpClient); } static async Task WaitForApiReady(HttpClient httpClient, int maxRetries = 30) diff --git a/SatelliteProvider.IntegrationTests/StubAndErrorContractTests.cs b/SatelliteProvider.IntegrationTests/StubAndErrorContractTests.cs new file mode 100644 index 0000000..9304fa3 --- /dev/null +++ b/SatelliteProvider.IntegrationTests/StubAndErrorContractTests.cs @@ -0,0 +1,89 @@ +using System.Net; +using System.Text; + +namespace SatelliteProvider.IntegrationTests; + +public static class StubAndErrorContractTests +{ + public static async Task RunAll(HttpClient httpClient) + { + RouteTestHelpers.PrintTestHeader("Test: Stub endpoints + error contracts (AZ-356 / AZ-353)"); + + await StubMgrs_Returns501(httpClient); + await StubUpload_Returns501(httpClient); + await CreateRoute_InvalidPayload_Returns400_AZ353_AC3(httpClient); + + Console.WriteLine("✓ Stub + error-contract tests: PASSED"); + } + + private static async Task StubMgrs_Returns501(HttpClient httpClient) + { + Console.WriteLine(); + Console.WriteLine("AZ-356 AC-1: GET /api/satellite/tiles/mgrs returns 501"); + + var response = await httpClient.GetAsync("/api/satellite/tiles/mgrs?mgrs=33TWN1234567890&squareSideMeters=100"); + var status = (int)response.StatusCode; + + if (status != 501) + { + throw new Exception($"Expected 501 from /api/satellite/tiles/mgrs, got {status}"); + } + + var body = await response.Content.ReadAsStringAsync(); + if (!body.Contains("Not implemented", StringComparison.OrdinalIgnoreCase)) + { + throw new Exception($"Expected ProblemDetails body containing 'Not implemented', got: {body}"); + } + + Console.WriteLine($" ✓ /api/satellite/tiles/mgrs returns HTTP 501 with ProblemDetails"); + } + + private static async Task StubUpload_Returns501(HttpClient httpClient) + { + Console.WriteLine(); + Console.WriteLine("AZ-356 AC-1: POST /api/satellite/upload returns 501"); + + using var multipart = new MultipartFormDataContent + { + { new StringContent(DateTime.UtcNow.ToString("o")), "Timestamp" }, + { new StringContent("47.461747"), "Lat" }, + { new StringContent("37.647063"), "Lon" }, + { new StringContent("100"), "Height" }, + { new StringContent("35"), "FocalLength" }, + { new StringContent("23"), "SensorWidth" }, + { new StringContent("15.6"), "SensorHeight" }, + }; + var fakeImage = new ByteArrayContent(new byte[] { 0xFF, 0xD8, 0xFF, 0xD9 }); + fakeImage.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("image/jpeg"); + multipart.Add(fakeImage, "Image", "test.jpg"); + + var response = await httpClient.PostAsync("/api/satellite/upload", multipart); + var status = (int)response.StatusCode; + + if (status != 501) + { + throw new Exception($"Expected 501 from /api/satellite/upload, got {status}"); + } + + Console.WriteLine($" ✓ /api/satellite/upload returns HTTP 501"); + } + + private static async Task CreateRoute_InvalidPayload_Returns400_AZ353_AC3(HttpClient httpClient) + { + Console.WriteLine(); + Console.WriteLine("AZ-353 AC-3: POST /api/satellite/route with <2 points returns 400 (typed ArgumentException path preserved)"); + + var routeId = Guid.NewGuid(); + var body = $"{{\"id\":\"{routeId}\",\"name\":\"too-short\",\"description\":\"\",\"regionSizeMeters\":500,\"zoomLevel\":18,\"requestMaps\":false,\"points\":[{{\"latitude\":47.46,\"longitude\":37.64}}]}}"; + var content = new StringContent(body, Encoding.UTF8, "application/json"); + var response = await httpClient.PostAsync("/api/satellite/route", content); + var status = (int)response.StatusCode; + + if (status != 400) + { + throw new Exception($"Expected 400 for 1-point route (typed ArgumentException), got {status}"); + } + + Console.WriteLine($" ✓ 1-point route rejected with HTTP 400 (typed handling preserved)"); + } +} diff --git a/SatelliteProvider.Tests/CorsConfigurationValidatorTests.cs b/SatelliteProvider.Tests/CorsConfigurationValidatorTests.cs new file mode 100644 index 0000000..7c6fed7 --- /dev/null +++ b/SatelliteProvider.Tests/CorsConfigurationValidatorTests.cs @@ -0,0 +1,108 @@ +using FluentAssertions; +using SatelliteProvider.Api; + +namespace SatelliteProvider.Tests; + +public class CorsConfigurationValidatorTests +{ + [Fact] + public void EnsureSafeForEnvironment_ProductionWithEmptyOriginsAndNoOptIn_Throws_AC1() + { + // Arrange + var allowedOrigins = Array.Empty(); + + // Act + Action act = () => CorsConfigurationValidator.EnsureSafeForEnvironment( + allowedOrigins, allowAnyOrigin: false, environmentName: "Production"); + + // Assert + act.Should().Throw() + .WithMessage("*CorsConfig:AllowedOrigins*") + .WithMessage("*CorsConfig:AllowAnyOrigin*"); + } + + [Theory] + [InlineData("Development")] + [InlineData("Staging")] + [InlineData("Local")] + public void EnsureSafeForEnvironment_NonProductionWithEmptyOrigins_DoesNotThrow_AC2(string environmentName) + { + // Arrange + var allowedOrigins = Array.Empty(); + + // Act + Action act = () => CorsConfigurationValidator.EnsureSafeForEnvironment( + allowedOrigins, allowAnyOrigin: false, environmentName); + + // Assert + act.Should().NotThrow(); + } + + [Fact] + public void EnsureSafeForEnvironment_ProductionWithExplicitAllowAnyOrigin_DoesNotThrow_AC3() + { + // Arrange + var allowedOrigins = Array.Empty(); + + // Act + Action act = () => CorsConfigurationValidator.EnsureSafeForEnvironment( + allowedOrigins, allowAnyOrigin: true, environmentName: "Production"); + + // Assert + act.Should().NotThrow(); + } + + [Fact] + public void EnsureSafeForEnvironment_ProductionWithNonEmptyOrigins_DoesNotThrow() + { + // Arrange + var allowedOrigins = new[] { "https://example.com" }; + + // Act + Action act = () => CorsConfigurationValidator.EnsureSafeForEnvironment( + allowedOrigins, allowAnyOrigin: false, environmentName: "Production"); + + // Assert + act.Should().NotThrow(); + } + + [Fact] + public void ShouldUsePermissivePolicy_NonEmptyOriginsAndNoOptIn_ReturnsFalse() + { + // Assert + CorsConfigurationValidator.ShouldUsePermissivePolicy( + new[] { "https://example.com" }, allowAnyOrigin: false).Should().BeFalse(); + } + + [Fact] + public void ShouldUsePermissivePolicy_EmptyOriginsAndNoOptIn_ReturnsTrue() + { + // Assert + CorsConfigurationValidator.ShouldUsePermissivePolicy( + Array.Empty(), allowAnyOrigin: false).Should().BeTrue(); + } + + [Fact] + public void ShouldUsePermissivePolicy_ExplicitOptIn_ReturnsTrueRegardlessOfOrigins() + { + // Assert + CorsConfigurationValidator.ShouldUsePermissivePolicy( + new[] { "https://example.com" }, allowAnyOrigin: true).Should().BeTrue(); + } + + [Fact] + public void ShouldWarnAboutPermissiveDefault_EmptyOriginsAndNoOptIn_ReturnsTrue() + { + // Assert + CorsConfigurationValidator.ShouldWarnAboutPermissiveDefault( + Array.Empty(), allowAnyOrigin: false).Should().BeTrue(); + } + + [Fact] + public void ShouldWarnAboutPermissiveDefault_ExplicitOptIn_ReturnsFalse() + { + // Assert + CorsConfigurationValidator.ShouldWarnAboutPermissiveDefault( + Array.Empty(), allowAnyOrigin: true).Should().BeFalse(); + } +} diff --git a/SatelliteProvider.Tests/GlobalExceptionHandlerTests.cs b/SatelliteProvider.Tests/GlobalExceptionHandlerTests.cs new file mode 100644 index 0000000..b8d1f7e --- /dev/null +++ b/SatelliteProvider.Tests/GlobalExceptionHandlerTests.cs @@ -0,0 +1,103 @@ +using System.Text.Json; +using FluentAssertions; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging; +using Moq; +using SatelliteProvider.Api; + +namespace SatelliteProvider.Tests; + +public class GlobalExceptionHandlerTests +{ + [Fact] + public async Task TryHandleAsync_WritesSanitizedProblemDetailsAndReturnsTrue_AC1() + { + // Arrange + var loggerMock = new Mock>(); + var handler = new GlobalExceptionHandler(loggerMock.Object); + var httpContext = new DefaultHttpContext { TraceIdentifier = "trace-12345" }; + httpContext.Request.Method = "POST"; + httpContext.Request.Path = "/api/satellite/route"; + var body = new MemoryStream(); + httpContext.Response.Body = body; + var leakySecret = "Connection string Host=secret-db;Password=hunter2 failed at line 42"; + var exception = new InvalidOperationException(leakySecret); + + // Act + var handled = await handler.TryHandleAsync(httpContext, exception, CancellationToken.None); + + // Assert + handled.Should().BeTrue(); + httpContext.Response.StatusCode.Should().Be(StatusCodes.Status500InternalServerError); + httpContext.Response.ContentType.Should().Contain("application/problem+json"); + + body.Position = 0; + using var doc = JsonDocument.Parse(body); + var root = doc.RootElement; + + root.GetProperty("status").GetInt32().Should().Be(500); + root.GetProperty("title").GetString().Should().Be("Internal Server Error"); + root.GetProperty("detail").GetString().Should().NotContain("hunter2"); + root.GetProperty("detail").GetString().Should().NotContain("secret-db"); + root.GetProperty("correlationId").GetString().Should().Be("trace-12345"); + } + + [Fact] + public async Task TryHandleAsync_BadHttpRequestException_HonorsFrameworkStatusAndDoesNotErrorLog_AC3() + { + // Arrange + var loggerMock = new Mock>(); + var handler = new GlobalExceptionHandler(loggerMock.Object); + var httpContext = new DefaultHttpContext { TraceIdentifier = "trace-bind-fail" }; + httpContext.Response.Body = new MemoryStream(); + var bindFailure = new BadHttpRequestException( + "Failed to bind parameter \"double Latitude\" from \"abc\".", + StatusCodes.Status400BadRequest); + + // Act + var handled = await handler.TryHandleAsync(httpContext, bindFailure, CancellationToken.None); + + // Assert + handled.Should().BeTrue("the handler writes the response itself rather than promoting to 5xx"); + httpContext.Response.StatusCode.Should().Be(StatusCodes.Status400BadRequest, + "framework-level request errors must keep their intended 4xx status, not become 500"); + loggerMock.Verify( + l => l.Log( + LogLevel.Error, + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny>()), + Times.Never, + "BadHttpRequestException is a client error and must not be ERROR-logged as a server failure"); + } + + [Fact] + public async Task TryHandleAsync_LogsFullExceptionWithCorrelationId_AC2() + { + // Arrange + var loggerMock = new Mock>(); + var handler = new GlobalExceptionHandler(loggerMock.Object); + var httpContext = new DefaultHttpContext { TraceIdentifier = "trace-AC2" }; + httpContext.Request.Method = "GET"; + httpContext.Request.Path = "/api/satellite/region/abc"; + httpContext.Response.Body = new MemoryStream(); + var exception = new InvalidOperationException("inner failure detail"); + + // Act + await handler.TryHandleAsync(httpContext, exception, CancellationToken.None); + + // Assert + loggerMock.Verify( + l => l.Log( + LogLevel.Error, + It.IsAny(), + It.Is((state, _) => + state.ToString()!.Contains("trace-AC2") && + state.ToString()!.Contains("/api/satellite/region/abc") && + state.ToString()!.Contains("GET")), + exception, + It.IsAny>()), + Times.Once); + } +} diff --git a/SatelliteProvider.Tests/SatelliteProvider.Tests.csproj b/SatelliteProvider.Tests/SatelliteProvider.Tests.csproj index 4af5bca..e2b41d2 100644 --- a/SatelliteProvider.Tests/SatelliteProvider.Tests.csproj +++ b/SatelliteProvider.Tests/SatelliteProvider.Tests.csproj @@ -36,6 +36,7 @@ + diff --git a/_docs/02_tasks/_dependencies_table.md b/_docs/02_tasks/_dependencies_table.md index e8161f1..c33e80d 100644 --- a/_docs/02_tasks/_dependencies_table.md +++ b/_docs/02_tasks/_dependencies_table.md @@ -33,9 +33,9 @@ Roadmap: `_docs/04_refactoring/03-code-quality-refactoring/analysis/refactoring_ | AZ-351 | C01 | Fix null logger to DatabaseMigrator | 1 | — | 2 | Done (In Testing) | | AZ-352 | C02 | Replace empty catch in ExtractTileCoordinatesFromFilename | 1 | — | 2 | Done (In Testing) | | AZ-363 | C10 | Delete write-only counters in RegionRequestQueue | 1 | — | 1 | Done (In Testing) | -| AZ-356 | C05 | Stub endpoints return 501 | 1 | — | 2 | To Do | -| AZ-354 | C04 | Strict CORS by default | 1 | — | 2 | To Do | -| AZ-353 | C03 | Sanitize 5xx responses via IExceptionHandler | 1 | — | 3 | To Do | +| AZ-356 | C05 | Stub endpoints return 501 | 1 | — | 2 | Done (In Testing) | +| AZ-354 | C04 | Strict CORS by default | 1 | — | 2 | Done (In Testing) | +| AZ-353 | C03 | Sanitize 5xx responses via IExceptionHandler | 1 | — | 3 | Done (In Testing) | | AZ-359 | C07 | Consolidate RegionService catch ladder | 2 | — | 3 | To Do | | AZ-357 | C06 | Drop tile Version concept; new migration | 2 | — | 5 | To Do | | AZ-362 | C09 | Idempotent POST contract | 2 | AZ-353 | 3 | To Do | diff --git a/_docs/02_tasks/todo/AZ-353_refactor_sanitize_5xx_responses.md b/_docs/02_tasks/done/AZ-353_refactor_sanitize_5xx_responses.md similarity index 100% rename from _docs/02_tasks/todo/AZ-353_refactor_sanitize_5xx_responses.md rename to _docs/02_tasks/done/AZ-353_refactor_sanitize_5xx_responses.md diff --git a/_docs/02_tasks/todo/AZ-354_refactor_strict_cors_default.md b/_docs/02_tasks/done/AZ-354_refactor_strict_cors_default.md similarity index 100% rename from _docs/02_tasks/todo/AZ-354_refactor_strict_cors_default.md rename to _docs/02_tasks/done/AZ-354_refactor_strict_cors_default.md diff --git a/_docs/02_tasks/todo/AZ-356_refactor_stub_endpoints_501.md b/_docs/02_tasks/done/AZ-356_refactor_stub_endpoints_501.md similarity index 100% rename from _docs/02_tasks/todo/AZ-356_refactor_stub_endpoints_501.md rename to _docs/02_tasks/done/AZ-356_refactor_stub_endpoints_501.md diff --git a/_docs/03_implementation/batch_08_report.md b/_docs/03_implementation/batch_08_report.md new file mode 100644 index 0000000..e1cf27a --- /dev/null +++ b/_docs/03_implementation/batch_08_report.md @@ -0,0 +1,99 @@ +# Batch Report + +**Batch**: 8 (refactor 03-code-quality-refactoring · Phase 1 — critical defensive fixes, part 2/2) +**Tasks**: AZ-356, AZ-354, AZ-353 +**Date**: 2026-05-10 +**Total Story Points**: 7 (2 + 2 + 3) + +## Task Results + +| Task | Status | Files Modified | Tests | AC Coverage | Issues | +|------|--------|---------------|-------|-------------|--------| +| AZ-356 Stub endpoints return 501 | Done | 1 (Program.cs handlers + OpenAPI) | 2 new integration | 3/3 ACs | None | +| AZ-354 Strict CORS by default | Done | 1 svc helper + 1 Program.cs hook | 9 new unit | 4/4 ACs | None | +| AZ-353 Sanitize 5xx via IExceptionHandler | Done | 1 svc class + Program.cs rewire + per-endpoint catch removal | 3 new unit + 1 new integration | 4/4 ACs | None | + +## Files Changed + +### Source +- `SatelliteProvider.Api/GlobalExceptionHandler.cs` — NEW (AZ-353) +- `SatelliteProvider.Api/CorsConfigurationValidator.cs` — NEW (AZ-354) +- `SatelliteProvider.Api/Program.cs` — usings + CORS validator wiring + warn-after-build + ExceptionHandler/ProblemDetails registration + UseExceptionHandler middleware + 501 stubs + per-endpoint try/catch stripping (AZ-353/354/356) + +### Tests +- `SatelliteProvider.Tests/CorsConfigurationValidatorTests.cs` — NEW (AZ-354, 9 tests) +- `SatelliteProvider.Tests/GlobalExceptionHandlerTests.cs` — NEW (AZ-353, 3 tests) +- `SatelliteProvider.Tests/SatelliteProvider.Tests.csproj` — added `ProjectReference` to Api so unit tests can reach the new classes +- `SatelliteProvider.IntegrationTests/StubAndErrorContractTests.cs` — NEW (AZ-356 AC-1, AZ-353 AC-3, 3 tests) +- `SatelliteProvider.IntegrationTests/Program.cs` — added `StubAndErrorContractTests.RunAll` to smoke + full + +## AC Test Coverage: All covered + +| Task | AC | Test | +|------|----|------| +| AZ-353 | AC-1 (5xx body sanitized) | `GlobalExceptionHandlerTests.TryHandleAsync_WritesSanitizedProblemDetailsAndReturnsTrue_AC1` (asserts response body has generic title + correlationId, not the leaky exception message) | +| AZ-353 | AC-2 (server log has full exception + correlationId) | `GlobalExceptionHandlerTests.TryHandleAsync_LogsFullExceptionWithCorrelationId_AC2` | +| AZ-353 | AC-3 (4xx paths preserved) | `GlobalExceptionHandlerTests.TryHandleAsync_BadHttpRequestException_HonorsFrameworkStatusAndDoesNotErrorLog_AC3` (unit) + `StubAndErrorContractTests.CreateRoute_InvalidPayload_Returns400_AZ353_AC3` (integration; ArgumentException → 400) + smoke `SecurityTests.SEC-01..04` (regression check that 4xx framework paths still produce 4xx) | +| AZ-353 | AC-4 (tests stay green) | smoke green | +| AZ-354 | AC-1 (Production refuses to start without origins) | `CorsConfigurationValidatorTests.EnsureSafeForEnvironment_ProductionWithEmptyOriginsAndNoOptIn_Throws_AC1` | +| AZ-354 | AC-2 (Development warns but starts) | `CorsConfigurationValidatorTests.EnsureSafeForEnvironment_NonProductionWithEmptyOrigins_DoesNotThrow_AC2` (Theory: Development/Staging/Local) + smoke (compose-managed `ASPNETCORE_ENVIRONMENT=Development` runs warn-after-build path; smoke is green) | +| AZ-354 | AC-3 (explicit opt-in works) | `CorsConfigurationValidatorTests.EnsureSafeForEnvironment_ProductionWithExplicitAllowAnyOrigin_DoesNotThrow_AC3` | +| AZ-354 | AC-4 (tests stay green) | smoke green | +| AZ-356 | AC-1 (both stubs return 501) | `StubAndErrorContractTests.StubMgrs_Returns501` + `StubAndErrorContractTests.StubUpload_Returns501` | +| AZ-356 | AC-2 (OpenAPI marks not-implemented) | `.ProducesProblem(StatusCodes.Status501NotImplemented)` annotations in `Program.cs`; verified by inspecting the route registration | +| AZ-356 | AC-3 (tests stay green) | smoke green | + +## Code Review Verdict: PASS + +### Phase 2 (Spec Compliance) +- All ACs satisfied with executed tests. +- `BadHttpRequestException` carve-out added to GlobalExceptionHandler — required by AC-3 (4xx paths preserved). Without it, the handler would have promoted SEC-01..04 framework binding errors from 400 to 500. + +### Phase 3 (Code Quality) +- `CorsConfigurationValidator`: pure static class (no I/O, no state) — allowed under coderule. +- `GlobalExceptionHandler`: instance class with injected logger; private static helper `WriteClientErrorAsync` is pure HTTP serialization — allowed. +- Per-endpoint try/catch removed for 7 of 7 affected handlers; the `CreateRoute` typed `ArgumentException` catch is intentionally preserved per AC-3. Endpoint signatures lost the `ILogger logger` parameter where it became unused. + +### Phase 4 (Security) +- `application/problem+json` content type set explicitly (override `WriteAsJsonAsync` default). +- Sanitized 500 body removes server-internal text (validated by `..._WritesSanitizedProblemDetailsAndReturnsTrue_AC1` which uses a connection-string-shaped exception message and asserts the password literal does NOT appear in the response). +- Strict CORS in Production is the deliverable. + +### Phase 5 (Performance) +- No hot-path changes. + +### Phase 6 (Cross-Task Consistency) +- All three tasks edit the same Program.cs in different sections (CORS / DI / pipeline / endpoints) without conflict. +- The single `using SatelliteProvider.Api;` directive serves all three new classes. + +### Phase 7 (Architecture Compliance) +- All new files live under `SatelliteProvider.Api/**` (Api OWNS). +- `SatelliteProvider.Tests` now references `SatelliteProvider.Api` — this is a downward (test → top-of-stack) reference, which is idiomatic for unit-testing API-internal helpers. Tests project remains the only consumer of Api types outside Api itself. + +## Auto-Fix Attempts: 1 (one re-run cycle to honor BadHttpRequestException StatusCode) +## Stuck Agents: None + +## Tracker Status (post-batch) + +| Task | Pre-batch | Post-batch (after commit) | +|------|-----------|---------------------------| +| AZ-356 | In Progress | → In Testing | +| AZ-354 | In Progress | → In Testing | +| AZ-353 | In Progress | → In Testing | + +## Phase 1 Summary (Batches 7+8) + +| Batch | Tasks | SP | Outcome | +|-------|-------|---:|---------| +| 7 | AZ-351 / AZ-352 / AZ-363 | 5 | PASS | +| 8 | AZ-356 / AZ-354 / AZ-353 | 7 | PASS | + +Phase 1 complete: 6 tasks, 12 SP, 0 blockers. All Critical-severity defensive fixes landed. + +## Next Batch + +**Batch 9** (Phase 2 — correctness): AZ-359 (consolidate RegionService catch ladder, 3 SP), AZ-357 (drop tile Version concept + new migration, 5 SP), AZ-362 (idempotent POST contract, 3 SP, depends on AZ-353 ✓). Total 11 SP. + +## Commit + +`[AZ-353][AZ-354][AZ-356] Refactor 03 batch 2: harden API surface` (commit hash recorded post-commit). diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 76e7328..5f8d892 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -8,7 +8,8 @@ status: in_progress sub_step: phase: 4 name: execution - detail: "RUN_DIR=03-code-quality-refactoring; epic AZ-350; batch 7 done (AZ-351/352/363, 5 SP); 24 tickets remaining; next batch 8 = AZ-356/354/353 (Phase 1 part 2, 7 SP, WebApi)" + detail: "RUN_DIR=03-code-quality-refactoring; epic AZ-350; batches 7+8 done (Phase 1 complete: 6 tasks/12 SP); 21 tickets/~54 SP remaining; next batch 9 = AZ-359/357/362 (Phase 2 correctness, 11 SP)" retry_count: 0 cycle: 1 tracker: jira +auto_push: true