mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 17:41:15 +00:00
[AZ-808] [AZ-811] Strict validation on region POST + lat/lon GET
AZ-808: FluentValidation for POST /api/satellite/request - RegionRequestValidator: id non-empty, lat/lon/sizeMeters/zoomLevel ranges - RequestRegionRequest: [JsonRequired] on every property, no implicit defaults - Wired via .WithValidation<RequestRegionRequest>() in MapPost chain - Unit + integration tests + curl probe script - New contract: contracts/api/region-request.md v1.0.0 AZ-811: FluentValidation + envelope filter for GET /api/satellite/tiles/latlon - GetTileByLatLonQuery: nullable record (double?/int?) so the minimal-API binder never short-circuits with BadHttpRequestException before filters - GetTileByLatLonQueryValidator: Cascade(Stop) + NotNull + InclusiveBetween per param; missing surfaces as `\`<name>\` is required.` - RejectUnknownQueryParamsEndpointFilter: reusable IEndpointFilter that rejects any query key outside the allowed set with errors[<key>] map; catches legacy `?Latitude=` typos and hostile probes (`?debug=1&admin=1`) - Handler: [AsParameters] GetTileByLatLonQuery + .Value deref post-validator - Unit (validator + filter) + integration tests + curl probe script - New contract: contracts/api/tile-latlon.md v1.0.0 Shared hygiene - Promote AssertErrorsContainsMention from per-test-file private helpers to ProblemDetailsAssertions (closes batch-1 Low-severity DRY warning) - Sync Swagger param descriptions, README, blackbox/security/perf scripts, uuidv5 doc with the new lat/lon/zoom query-param names Docs - system-flows.md F1/F2 reference the new contracts + validation layers - modules/api_program.md adds Api/Validators + Api/DTOs sections - _autodev_state.md: batch 2 of 4 complete; next batch = AZ-809 All smoke tests green (mode=smoke, exit 0). AZ-808 + AZ-811 transitioned to In Testing on Jira. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,151 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 02 (cycle 8)
|
||||
**Tasks**: AZ-808 (Region POST endpoint strict validation) + AZ-811 (lat/lon GET endpoint strict validation)
|
||||
**Date**: 2026-05-22
|
||||
**Verdict**: PASS_WITH_NOTES
|
||||
|
||||
## Findings
|
||||
|
||||
| # | Severity | Category | File:Line | Title |
|
||||
|---|----------|----------|-----------|-------|
|
||||
| 1 | Info | Design rationale | `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs:1-30` | DTO uses `double?` / `int?` on purpose to dodge minimal-API "Required parameter not provided" short-circuit |
|
||||
|
||||
### Finding Details
|
||||
|
||||
**F1: `GetTileByLatLonQuery` uses nullable types on purpose** (Info / Design rationale)
|
||||
- Location: `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs:17-20`
|
||||
- Description: The DTO declares `Lat`/`Lon`/`Zoom` as `double?`/`double?`/`int?`. Non-nullable variants would feel simpler but cause the minimal-API parameter binder to throw `BadHttpRequestException` BEFORE endpoint filters run when a query param is missing. That short-circuit produces a plain `ProblemDetails` via `GlobalExceptionHandler` — no `errors{}` envelope, no per-field key — which violates AZ-811 ACs 1 and 4 (every failure mode must surface as `errors.<paramName>`).
|
||||
- Initial implementation used non-nullable types. Diagnostic instrumentation captured the failing test response body (`{"title":"Bad Request","status":400,"detail":"Required parameter \"double Lat\" was not provided from query string."}`) which proved the binder was short-circuiting. Fix: switch to nullable + add `NotNull()` rule in the validator with `CascadeMode.Stop` ahead of the range rule. The handler dereferences `.Value` only after the validator filter passes.
|
||||
- Suggestion: NONE — the rationale is now documented in both the DTO XML/doc comment and `api_program.md::Api/DTOs`. Captured here so a future reader doesn't "simplify" the types back to non-nullable.
|
||||
- Task: AZ-811
|
||||
|
||||
## Phase Summary
|
||||
|
||||
| Phase | Outcome |
|
||||
|-------|---------|
|
||||
| 1. Context Loading | Read AZ-808 + AZ-811 specs, `_docs/02_document/contracts/api/tile-inventory.md` (validator pattern reference from cycle 7), and the cycle-7 `ValidationEndpointFilter<T>` shared infra. Both tasks share batch 2 because both wire `WithValidation<T>()` and reuse the cycle-7 validation envelope. |
|
||||
| 2. Spec Compliance | **AZ-808**: AC-1..AC-8 all ✓. New `RegionRequestValidator` covers `id`/`lat`/`lon`/`sizeMeters`/`zoomLevel`. `[JsonRequired]` on `RequestRegionRequest` enforces required-field at the deserializer (no defaulting). New contract `region-request.md` v1.0.0 published. Unit + integration tests cover happy path + each rule + missing-required + type-mismatch. Probe script exercises every failure mode via `curl`. **AZ-811**: AC-1..AC-9 all ✓. New `GetTileByLatLonQueryValidator` covers `lat`/`lon`/`zoom` with explicit `NotNull` for missing + `InclusiveBetween` for range (CascadeMode.Stop). New `RejectUnknownQueryParamsEndpointFilter` rejects any query key outside `{lat, lon, zoom}` with `Results.ValidationProblem`. New contract `tile-latlon.md` v1.0.0 published. Unit tests for both validator (7 methods) and filter (4 methods); integration tests cover happy path + 6 failure modes. Probe script exercises every failure mode. |
|
||||
| 3. Code Quality | Mechanical patterns followed; new validators and filter are minimal and SRP-clean. One Info finding (F1) on the nullable-DTO design — surfaced rather than left implicit. Cycle-7 `AssertErrorsContainsMention` helper promoted to `ProblemDetailsAssertions.cs` (closes the Low-severity DRY warning from batch-1 review). |
|
||||
| 4. Security | `RejectUnknownQueryParamsEndpointFilter` rejects fingerprinting probes (`?debug=1&admin=true`, `?Latitude=...&Longitude=...`) with HTTP 400 + named keys — no enumeration vector. `RegionRequestValidator` runs BEFORE any DB work (idempotency lookup, queueing). No SQL injection vectors, no hardcoded secrets, no PII in logs. JWT auth retained on both endpoints. |
|
||||
| 5. Performance | Validators run synchronously against in-memory record fields — negligible cost vs the Google-Maps round-trip or DB write that follows. Endpoint filter inspects `Query.Keys` (in-memory dictionary scan). No N+1, no blocking I/O. |
|
||||
| 6. Cross-Task Consistency | Both tasks share `ValidationEndpointFilter<T>` infra from cycle 7 and the new shared `ProblemDetailsAssertions.AssertErrorsContainsMention`. `RegionRequestValidator` and `GetTileByLatLonQueryValidator` follow the same `Cascade(Stop).NotNull().InclusiveBetween()` pattern. Both produce identically-shaped `ValidationProblemDetails` per `error-shape.md` v1.0.0. |
|
||||
| 7. Architecture Compliance | DTOs in `SatelliteProvider.Common/DTO/` (Region) and `SatelliteProvider.Api/DTOs/` (latlon query) — the query record is API-local because its `[FromQuery]` binding semantics are not reusable outside the API layer. Validators co-located with the API at `SatelliteProvider.Api/Validators/`. No layering violations. No cycles, no public-API bypasses, no ADR breaches. |
|
||||
|
||||
## Files Reviewed
|
||||
|
||||
### AZ-808 (Region POST validator)
|
||||
- `SatelliteProvider.Common/DTO/RequestRegionRequest.cs` — `[JsonRequired]` on every property; removed default values for `ZoomLevel` and `StitchTiles` so callers cannot rely on implicit defaults.
|
||||
- `SatelliteProvider.Api/Validators/RegionRequestValidator.cs` — **NEW** — 5 rules (id non-empty + 4 range rules).
|
||||
- `SatelliteProvider.Api/Program.cs` (lines around `MapPost("/api/satellite/request", ...)`) — added `.WithValidation<RequestRegionRequest>()`, `.Accepts<>`, `.Produces<>`, `.ProducesProblem()`. Removed inline `request.SizeMeters` size check (now in validator).
|
||||
- `SatelliteProvider.Tests/Validators/RegionRequestValidatorTests.cs` — **NEW** — Theory + Fact coverage for each rule, positive and negative.
|
||||
- `SatelliteProvider.IntegrationTests/RegionRequestValidationTests.cs` — **NEW** — Happy + empty body + missing/zero GUID + 4 out-of-range + missing-stitch + type mismatch.
|
||||
- `SatelliteProvider.IntegrationTests/Program.cs` — wired `RegionRequestValidationTests.RunAll` into smoke + full suites.
|
||||
- `scripts/probe_region_validation.sh` — **NEW** — curl probes for every failure mode.
|
||||
- `_docs/02_document/contracts/api/region-request.md` — **NEW** — v1.0.0 contract (no prior version existed).
|
||||
- `_docs/02_document/modules/api_program.md` — RequestRegion handler description updated; references new contract.
|
||||
- `_docs/02_document/system-flows.md::F2` — references new contract + validator.
|
||||
|
||||
### AZ-811 (lat/lon GET validator)
|
||||
- `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs` — **NEW** — nullable record with `[FromQuery(Name = ...)]` per property. Rationale documented in-file (F1).
|
||||
- `SatelliteProvider.Api/Validators/GetTileByLatLonQueryValidator.cs` — **NEW** — `Cascade(Stop).NotNull().InclusiveBetween` per param.
|
||||
- `SatelliteProvider.Api/Validators/RejectUnknownQueryParamsEndpointFilter.cs` — **NEW** — reusable `IEndpointFilter` parameterised by allowed-keys set (case-insensitive). Returns `Results.ValidationProblem` with one error per unknown key.
|
||||
- `SatelliteProvider.Api/Program.cs` (lines around `MapGet("/api/satellite/tiles/latlon", ...)`) — added envelope filter + `.WithValidation<GetTileByLatLonQuery>()`. Handler signature now `[AsParameters] GetTileByLatLonQuery query`; dereferences `query.Lat!.Value` etc.
|
||||
- `SatelliteProvider.Api/Swagger/ParameterDescriptionFilter.cs` — descriptions for `lat`/`lon`/`zoom` (post-rename); removed legacy `Latitude`/`Longitude`/`ZoomLevel` entries.
|
||||
- `SatelliteProvider.Tests/Validators/GetTileByLatLonQueryValidatorTests.cs` — **NEW** — 9 methods incl. null cases.
|
||||
- `SatelliteProvider.Tests/Validators/RejectUnknownQueryParamsEndpointFilterTests.cs` — **NEW** — 4 methods (delegation, unknown-key block, legacy PascalCase, case-insensitive allowed-set).
|
||||
- `SatelliteProvider.IntegrationTests/GetTileByLatLonValidationTests.cs` — **NEW** — Happy + 3 out-of-range + 1 missing-required + 2 unknown-key (legacy + hostile) + 1 type-mismatch = 8 methods.
|
||||
- `SatelliteProvider.IntegrationTests/Program.cs` — wired `GetTileByLatLonValidationTests.RunAll` into smoke + full suites.
|
||||
- `SatelliteProvider.IntegrationTests/TileTests.cs` — URL `?Latitude=&Longitude=&ZoomLevel=` → `?lat=&lon=&zoom=`.
|
||||
- `SatelliteProvider.IntegrationTests/JwtIntegrationTests.cs` — `ProtectedTilesPath` const updated.
|
||||
- `SatelliteProvider.IntegrationTests/SecurityTests.cs` — SQLi probe URL updated.
|
||||
- `scripts/probe_latlon_validation.sh` — **NEW** — curl probes incl. missing-lat, hostile probes, type mismatch.
|
||||
- `scripts/run-performance-tests.sh` — PT-01 URL updated.
|
||||
- `README.md` — endpoint example updated.
|
||||
- `_docs/02_document/contracts/api/tile-latlon.md` — **NEW** — v1.0.0 contract (no prior version existed).
|
||||
- `_docs/02_document/modules/api_program.md` — handler + `Api/Validators` + `Api/DTOs` sections updated.
|
||||
- `_docs/02_document/modules/common_uuidv5.md` — example URL updated.
|
||||
- `_docs/02_document/system-flows.md::F1` — references new contract + validation layers.
|
||||
- `_docs/02_document/tests/blackbox-tests.md` — BT-01, BT-N01, BT-N02, BT-18 triggers updated.
|
||||
- `_docs/02_document/tests/security-tests.md` — SEC-01, SEC-05 triggers updated.
|
||||
|
||||
### Shared (cross-task hygiene)
|
||||
- `SatelliteProvider.IntegrationTests/ProblemDetailsAssertions.cs` — `AssertErrorsContainsMention` promoted from per-test-file private helper to public static. Closes batch-1 Low-severity DRY warning.
|
||||
- `SatelliteProvider.IntegrationTests/TileInventoryValidationTests.cs` — uses shared helper.
|
||||
- `SatelliteProvider.IntegrationTests/RegionFieldRenameTests.cs` — uses shared helper; removed unused `using` directives.
|
||||
|
||||
## Test Evidence
|
||||
|
||||
`./scripts/run-tests.sh --smoke` (`integration-tests` container exit code 0):
|
||||
|
||||
```
|
||||
Test: POST /api/satellite/request strict validation (AZ-808)
|
||||
============================================================
|
||||
|
||||
AZ-808 AC-2: well-formed body → HTTP 200
|
||||
✓ {id,lat,lon,sizeMeters,zoomLevel,stitchTiles} accepted with HTTP 200
|
||||
|
||||
AZ-808 rule (id-empty): id=Guid.Empty → HTTP 400
|
||||
✓ id=Guid.Empty rejected with errors["id"]
|
||||
|
||||
AZ-808 rule (id-missing): missing id → HTTP 400 via [JsonRequired]
|
||||
✓ Missing id rejected via [JsonRequired] (no defaulting to Guid.Empty)
|
||||
|
||||
AZ-808 rule (lat-out-of-range): lat=91 → HTTP 400
|
||||
✓ lat=91 rejected with errors["lat"]
|
||||
|
||||
AZ-808 rule (lon-out-of-range): lon=181 → HTTP 400
|
||||
✓ lon=181 rejected with errors["lon"]
|
||||
|
||||
AZ-808 rule (sizeMeters-out-of-range): sizeMeters=50 → HTTP 400
|
||||
✓ sizeMeters=50 rejected with errors["sizeMeters"]
|
||||
|
||||
AZ-808 rule (zoomLevel-out-of-range): zoomLevel=30 → HTTP 400
|
||||
✓ zoomLevel=30 rejected with errors["zoomLevel"]
|
||||
|
||||
AZ-808 rule (stitchTiles-missing): missing stitchTiles → HTTP 400 via [JsonRequired]
|
||||
✓ Missing stitchTiles rejected via [JsonRequired]
|
||||
|
||||
AZ-808 rule (type-mismatch): lat="bad" → HTTP 400
|
||||
✓ Non-numeric lat rejected with HTTP 400
|
||||
|
||||
AZ-808 empty body → HTTP 400
|
||||
✓ Empty body rejected with HTTP 400
|
||||
✓ POST /api/satellite/request validation tests: PASSED
|
||||
|
||||
Test: GET /api/satellite/tiles/latlon strict validation (AZ-811)
|
||||
================================================================
|
||||
|
||||
AZ-811 AC-2: well-formed query → HTTP 200
|
||||
✓ {lat,lon,zoom} accepted with HTTP 200
|
||||
|
||||
AZ-811 rule 1: lat out of range (-90..90) → HTTP 400
|
||||
✓ lat=91 rejected with errors["lat"]
|
||||
|
||||
AZ-811 rule 2: lon out of range (-180..180) → HTTP 400
|
||||
✓ lon=181 rejected with errors["lon"]
|
||||
|
||||
AZ-811 rule 3: zoom out of range (0..22) → HTTP 400
|
||||
✓ zoom=30 rejected with errors["zoom"]
|
||||
|
||||
AZ-811 rule 1: missing `lat` query param → HTTP 400 with errors.lat
|
||||
✓ Missing lat rejected with errors["lat"] = `lat` is required
|
||||
|
||||
AZ-811 rule 4: legacy `?Latitude=&Longitude=&ZoomLevel=` (pre-AZ-811 wire format) → HTTP 400 (envelope filter)
|
||||
✓ Legacy ?Latitude=&Longitude=&ZoomLevel= rejected by envelope filter
|
||||
|
||||
AZ-811 rule 4: hostile/typo query keys → HTTP 400 (envelope filter)
|
||||
✓ ?debug=1&admin=true rejected; errors map names BOTH unknown keys
|
||||
|
||||
AZ-811 rule 5: lat type mismatch (non-numeric) → HTTP 400
|
||||
✓ lat=fifty rejected with HTTP 400
|
||||
✓ GET lat/lon validation tests: PASSED
|
||||
```
|
||||
|
||||
`=== All tests passed (mode=smoke) ===` — no regressions in cycle-7 inventory/idempotent/security/route/tile/leaflet/migration suites.
|
||||
|
||||
## Verdict Logic
|
||||
|
||||
- No Critical, no High, no Medium findings.
|
||||
- 1 Info finding (F1) — design rationale captured in code + doc; not a regression.
|
||||
- **PASS_WITH_NOTES**.
|
||||
Reference in New Issue
Block a user