mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 23:41:14 +00:00
[AZ-353][AZ-354][AZ-356] Refactor 03 batch 2: harden API surface
AZ-353: Centralize 500 handling via GlobalExceptionHandler / AddProblemDetails / UseExceptionHandler. Sanitized ProblemDetails body carries a generic title, RFC9110 type link, and the request's TraceIdentifier as correlationId; the leaky exception message stays server-side in the ERR log entry. Strip per-endpoint try/catch (Exception) wrappers and the unused ILogger<Program> parameters they served. Preserve the typed ArgumentException catch in CreateRoute (AC-3). The handler maps BadHttpRequestException back to its framework-supplied StatusCode so model-binding / malformed-body failures stay 4xx instead of being promoted to 500. AZ-354: Extract CorsConfigurationValidator (pure static helpers) and wire it into Program.cs. Production with empty CorsConfig:AllowedOrigins and no CorsConfig:AllowAnyOrigin opt-in now throws InvalidOperationException at host startup. Development keeps the permissive default but logs a warning post-build. Adds the explicit CorsConfig:AllowAnyOrigin escape hatch. AZ-356: GetSatelliteTilesByMgrs and UploadImage now return Results.Problem(StatusCode 501) with ProblemDetails. Added .ProducesProblem(501) so swagger.json documents the not-implemented status. Tests: SatelliteProvider.Tests now references SatelliteProvider.Api (downward, idiomatic) so unit tests can reach the new helpers. +9 CorsConfigurationValidator unit tests, +3 GlobalExceptionHandler unit tests, +3 StubAndErrorContractTests integration tests (added to smoke + full suites). 58/58 unit + 5/5 smoke + 3/3 stub-contract pass. Code review verdict: PASS. Batch report: _docs/03_implementation/batch_08_report.md. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -1,74 +0,0 @@
|
||||
# Refactor: sanitize 5xx responses via global IExceptionHandler
|
||||
|
||||
**Task**: AZ-353_refactor_sanitize_5xx_responses
|
||||
**Name**: Centralized exception handler with sanitized ProblemDetails
|
||||
**Description**: Replace per-endpoint `try/catch (Exception)` + `Results.Problem(detail: ex.Message)` with a global `IExceptionHandler` that returns sanitized ProblemDetails (correlation ID + generic title) and logs the full exception server-side.
|
||||
**Complexity**: 3 points
|
||||
**Dependencies**: None
|
||||
**Component**: Api
|
||||
**Tracker**: AZ-353
|
||||
**Epic**: AZ-350
|
||||
|
||||
## Problem
|
||||
|
||||
`SatelliteProvider.Api/Program.cs` has six endpoint handlers (lines 139-143, 170-174, 206-210, 226-230, 245-249, 265-269) that each catch `Exception` and return `Results.Problem(detail: ex.Message, statusCode: 500)`. The `detail` ships the exception message — including stack-trace fragments, file paths, SQL error text, and Google API error bodies — back to the client.
|
||||
|
||||
## Outcome
|
||||
|
||||
- 5xx responses no longer leak internal exception messages.
|
||||
- Server-side logs contain the full exception + correlation ID for each 500.
|
||||
- Per-endpoint try/catch boilerplate is removed.
|
||||
- 37 unit + 5 smoke tests stay green (with assertions on `ProblemDetails.detail` updated).
|
||||
|
||||
## Scope
|
||||
|
||||
### Included
|
||||
- Add an `IExceptionHandler` (or `UseExceptionHandler` middleware) in `Program.cs`.
|
||||
- Generate a correlation ID per request, include it in both the response body and the server log entry.
|
||||
- Remove the per-endpoint catches that only re-emit `ex.Message`.
|
||||
- Update tests that assert on `ProblemDetails.detail` to assert on the sanitized shape.
|
||||
- Specific 400 paths (e.g., `ArgumentException` in `CreateRoute`) keep their typed handling.
|
||||
|
||||
### Excluded
|
||||
- Changing HTTP status codes for 4xx paths.
|
||||
- Adding new structured error categories (deferred).
|
||||
- Changing the logger sink configuration.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
**AC-1: 5xx body is sanitized**
|
||||
Given any endpoint that throws an unhandled exception
|
||||
When the client receives the 500 response
|
||||
Then `ProblemDetails.detail` does not contain the original exception message; the body has a generic title + correlation ID.
|
||||
|
||||
**AC-2: Server log has the full exception**
|
||||
Given the same scenario as AC-1
|
||||
When the application logs the failure
|
||||
Then the log entry contains the exception type, message, stack trace, and the same correlation ID returned to the client.
|
||||
|
||||
**AC-3: 4xx paths preserved**
|
||||
Given a request that triggers `ArgumentException` in `CreateRoute`
|
||||
When the endpoint runs
|
||||
Then the response is HTTP 400 with the existing typed shape (not 500).
|
||||
|
||||
**AC-4: Tests stay green**
|
||||
Given the post-refactor build
|
||||
When `scripts/run-tests.sh --smoke` runs
|
||||
Then all 37 unit + 5 smoke scenarios pass (with `ProblemDetails.detail` assertions updated).
|
||||
|
||||
## Constraints
|
||||
|
||||
- HTTP 500 status code preserved for unhandled exceptions.
|
||||
- No new dependencies beyond ASP.NET Core 8 built-ins.
|
||||
|
||||
## Risks & Mitigation
|
||||
|
||||
**Risk 1: tests that assert on `detail` text break**
|
||||
- *Risk*: existing unit/integration tests may inspect `ProblemDetails.detail`.
|
||||
- *Mitigation*: update them to assert on the new sanitized shape (title + correlationId) in the same PR.
|
||||
|
||||
**Risk 2: clients depend on the leaky message**
|
||||
- *Risk*: the API has been live; some integrator may parse the message.
|
||||
- *Mitigation*: this is a security improvement; document the change in the OpenAPI spec.
|
||||
|
||||
Full change entry: `_docs/04_refactoring/03-code-quality-refactoring/list-of-changes.md` (C03).
|
||||
@@ -1,66 +0,0 @@
|
||||
# Refactor: strict CORS by default; explicit opt-in for AllowAnyOrigin
|
||||
|
||||
**Task**: AZ-354_refactor_strict_cors_default
|
||||
**Name**: Strict CORS default + explicit opt-in for permissive policy
|
||||
**Description**: When `CorsConfig:AllowedOrigins` is empty, refuse to start in `Production` and warn loudly in `Development`. Only configure the open policy when the operator opts in via `CorsConfig:AllowAnyOrigin=true`.
|
||||
**Complexity**: 2 points
|
||||
**Dependencies**: None
|
||||
**Component**: Api
|
||||
**Tracker**: AZ-354
|
||||
**Epic**: AZ-350
|
||||
|
||||
## Problem
|
||||
|
||||
`SatelliteProvider.Api/Program.cs:37-47` falls through to `policy.AllowAnyOrigin().AllowAnyHeader().AllowAnyMethod()` when `CorsConfig:AllowedOrigins` is empty. A misconfigured prod deployment silently exposes the entire surface to any origin.
|
||||
|
||||
## Outcome
|
||||
|
||||
- Production deployment with empty `AllowedOrigins` fails to start with a clear error message.
|
||||
- Development deployment with empty `AllowedOrigins` logs a loud warning.
|
||||
- `CorsConfig:AllowAnyOrigin=true` (explicit) is the only path that produces the permissive policy.
|
||||
- 37 unit + 5 smoke tests stay green (test fixtures already specify origins).
|
||||
|
||||
## Scope
|
||||
|
||||
### Included
|
||||
- Read `CorsConfig:AllowAnyOrigin` (new boolean flag, default false).
|
||||
- Branch logic in `Program.cs` CORS configuration: empty origins + Production → throw; empty origins + Development → warn; non-empty origins → existing strict policy; explicit `AllowAnyOrigin=true` → existing permissive policy.
|
||||
- Update `appsettings.Development.json` to set explicit origins (or set the new flag) so dev still works out of the box.
|
||||
|
||||
### Excluded
|
||||
- Changing the CORS policy semantics for non-empty `AllowedOrigins`.
|
||||
- Adding per-endpoint CORS overrides.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
**AC-1: Production refuses to start without origins**
|
||||
Given `ASPNETCORE_ENVIRONMENT=Production` and empty `CorsConfig:AllowedOrigins` and no `CorsConfig:AllowAnyOrigin=true`
|
||||
When the host attempts to start
|
||||
Then it throws a clear configuration exception naming the missing setting.
|
||||
|
||||
**AC-2: Development warns but starts**
|
||||
Given `ASPNETCORE_ENVIRONMENT=Development` and empty `CorsConfig:AllowedOrigins`
|
||||
When the host starts
|
||||
Then a warning log entry is emitted and the host continues to run.
|
||||
|
||||
**AC-3: Explicit opt-in works**
|
||||
Given `CorsConfig:AllowAnyOrigin=true`
|
||||
When the host starts
|
||||
Then the permissive CORS policy is configured (current behavior).
|
||||
|
||||
**AC-4: Tests stay green**
|
||||
Given the post-refactor build
|
||||
When `scripts/run-tests.sh --smoke` runs
|
||||
Then all 37 unit + 5 smoke scenarios pass.
|
||||
|
||||
## Constraints
|
||||
|
||||
- Existing test/dev fixtures that specify origins must continue to work without changes (other than appsettings overrides).
|
||||
|
||||
## Risks & Mitigation
|
||||
|
||||
**Risk 1: existing prod env vars don't have origins set**
|
||||
- *Risk*: if any deployed environment relies on the default-permissive behavior, it will break.
|
||||
- *Mitigation*: this is the security fix the change exists to provide. Document in deploy notes / runbook.
|
||||
|
||||
Full change entry: `_docs/04_refactoring/03-code-quality-refactoring/list-of-changes.md` (C04).
|
||||
@@ -1,59 +0,0 @@
|
||||
# Refactor: stub endpoints return 501 Not Implemented
|
||||
|
||||
**Task**: AZ-356_refactor_stub_endpoints_501
|
||||
**Name**: Stub endpoints respond with HTTP 501
|
||||
**Description**: Change `GetSatelliteTilesByMgrs` and `UploadImage` to return HTTP 501 with a problem-details body, and update OpenAPI metadata accordingly.
|
||||
**Complexity**: 2 points
|
||||
**Dependencies**: None
|
||||
**Component**: Api
|
||||
**Tracker**: AZ-356
|
||||
**Epic**: AZ-350
|
||||
|
||||
## Problem
|
||||
|
||||
`SatelliteProvider.Api/Program.cs:177-180` (`GetSatelliteTilesByMgrs`) returns 200 OK with an empty payload. `Program.cs:182-185` (`UploadImage`) returns 200 OK with `Success=false`. Clients can't distinguish "stubbed" from "valid empty result" or "real failure".
|
||||
|
||||
## Outcome
|
||||
|
||||
- Both stub endpoints respond with HTTP 501 and a ProblemDetails body indicating "feature not implemented".
|
||||
- OpenAPI document marks both endpoints as not-implemented.
|
||||
- 37 unit + 5 smoke tests stay green.
|
||||
|
||||
## Scope
|
||||
|
||||
### Included
|
||||
- Change handler return statements to `Results.Problem(statusCode: 501, title: "Not implemented", detail: <short description>)`.
|
||||
- Update Swagger / OpenAPI annotations to reflect 501.
|
||||
|
||||
### Excluded
|
||||
- Implementing the underlying functionality (out of scope for this run).
|
||||
- Removing the endpoints (the routes are documented contract surface).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
**AC-1: Both stubs return 501**
|
||||
Given a request to `GET /api/satellite/tiles/mgrs` or `POST /api/satellite/upload`
|
||||
When the endpoint executes
|
||||
Then the response is HTTP 501 with a ProblemDetails body.
|
||||
|
||||
**AC-2: OpenAPI marks them not-implemented**
|
||||
Given the generated `swagger.json`
|
||||
When inspected
|
||||
Then the two endpoints declare `501` as a documented response.
|
||||
|
||||
**AC-3: Tests stay green**
|
||||
Given the post-refactor build
|
||||
When `scripts/run-tests.sh --smoke` runs
|
||||
Then all 37 unit + 5 smoke scenarios pass (smoke does not exercise these endpoints).
|
||||
|
||||
## Constraints
|
||||
|
||||
- Endpoints stay registered (route shape preserved); only the response status + body change.
|
||||
|
||||
## Risks & Mitigation
|
||||
|
||||
**Risk 1: integrators may have probed the stubs and treated 200 as success**
|
||||
- *Risk*: any caller that received 200 OK with empty body and proceeded as if the operation succeeded will now see 501.
|
||||
- *Mitigation*: this is the fix the change exists to provide. Honest contract over polite-but-wrong success.
|
||||
|
||||
Full change entry: `_docs/04_refactoring/03-code-quality-refactoring/list-of-changes.md` (C05).
|
||||
Reference in New Issue
Block a user